Export schedule dispatch #5643

tjiang11 · 2018-09-12T06:47:47Z

Sets up automatic backups utilizing the GCP import export service, as datastore admin backups are being deprecated.

codecov-io · 2018-09-12T07:31:33Z

Codecov Report

Merging #5643 into develop will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5643      +/-   ##
===========================================
- Coverage    45.83%   45.79%   -0.05%     
===========================================
  Files          511      513       +2     
  Lines        29826    29872      +46     
  Branches      4514     4519       +5     
===========================================
+ Hits         13672    13679       +7     
- Misses       16154    16193      +39

Impacted Files	Coverage Δ
...s_and_skills_dashboard/TopicsAndSkillsDashboard.js	`9.52% <0%> (-4.43%)`	⬇️
...ploration_editor/feedback_tab/ThreadDataService.js	`20.38% <0%> (-0.37%)`	⬇️
...topics_and_skills_dashboard/SkillsListDirective.js	`2.32% <0%> (-0.31%)`	⬇️
...v/head/pages/creator_dashboard/CreatorDashboard.js	`20% <0%> (-0.26%)`	⬇️
...itor/subtopics_editor/SubtopicsListTabDirective.js	`0.96% <0%> (-0.18%)`	⬇️
...d/pages/skill_editor/SkillEditorNavbarDirective.js	`1.92% <0%> (-0.08%)`	⬇️
...ts/top_navigation_bar/TopNavigationBarDirective.js	`0.85% <0%> (-0.01%)`	⬇️
...shboard/TopicsAndSkillsDashboardNavbarDirective.js	`4.54% <0%> (ø)`	⬆️
...es/topic_editor/questions/QuestionsTabDirective.js	`0.86% <0%> (ø)`	⬆️
...plates/dev/head/components/SkillCreationService.js	`5.88% <0%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f112523...3a13290. Read the comment docs.

seanlip

Thanks @tjiang11! Looks pretty good, left a few comments.

seanlip · 2018-09-12T19:07:59Z

export/app.yaml

+  version: "latest"
+
+handlers:
+- url: /cloud-datastore-export


Should this handle URLs with stuff after /cloud-datastore-export too? Like, it should 404 on /cloud-datastore-export/1?

Asking because I noticed such URL requests get sent here by dispatch.yaml but then don't seem to get handled here if I'm reading correctly.

Hmm actually I think maybe dispatch.yaml just shouldn't include anything after /cloud-datastore-export, I don't see why it would need to. I'll change dispatch.yaml

seanlip · 2018-09-12T19:08:12Z

dispatch.yaml

@@ -0,0 +1,4 @@
+dispatch:
+  # Send requests for exports to the cloud datastore admin service.
+  - url: "*/cloud-datastore-export/*"


Maybe use underscores instead of hyphens, same as we do in other places.

seanlip · 2018-09-12T19:08:58Z

export/cloud_datastore_admin.py

+
+        output_url_prefix = self.request.get('output_url_prefix')
+        assert output_url_prefix and output_url_prefix.startswith('gs://')
+        if '/' not in output_url_prefix[5:]:


Where does 5: come from? Maybe make the origin of the "5" more explicit.

Quick question, this code came from the tutorial on the online docs. I forgot to ask about accreditation, since when I tried to accredit it to Google in the copyright notice, I couldn't pass the lint checks.

Ah, just leave a comment in the file with a link to the source.

added comment

seanlip · 2018-09-12T19:09:53Z

export/cloud_datastore_admin.py

+        }
+        headers = {
+            'Content-Type': 'application/json',
+            'Authorization': 'Bearer ' + access_token


Prefer interpolation: 'Bearer %s' % access_token

seanlip · 2018-09-12T19:10:42Z

export/cloud_datastore_admin.py

+                deadline=60,
+                headers=headers)
+            if result.status_code == httplib.OK:
+                logging.info(result.content)


That's going to be lots of logs, maybe log a summary at the end of how many data items were successfully exported instead.

How can you tell this is going to be a lot of logs?

It's one per request, right?

Wouldn't it just be one log then, from one request?

Sorry, you're right! I am not sure what I was thinking here, for some reason I thought there was a "for" loop. Please ignore this comment.

seanlip · 2018-09-12T19:11:18Z

export/cloud_datastore_admin.py

+import webapp2
+
+
+class Export(webapp2.RequestHandler):


ExportToCloudDatastoreHandler

seanlip · 2018-09-12T19:12:02Z

export/cloud_datastore_admin.py

+
+class Export(webapp2.RequestHandler):
+
+    def get(self):


Guard this by a cron header check? See cron.py for how to do this -- @acl_decorators.can_perform_cron_tasks

I'm having some trouble with this, when I try running the cron job it keeps saying it can't find the module core.domain? I tried putting an init.py in the export folder but it seems to say the same thing

Interesting, I wouldn't have expected that. I wonder whether it's because app engine is regarding export/ as its own folder that's isolated from the world.

Perhaps take a look at pythonpath in that file to see what's in the pythonpath, and contrast it to what you get when logging pythonpath in a file in core?

If they truly are separated then it's fine to add a decorators.py file in export/ as well.

I took a look into the pythonpath, and it does seem like it's different, since the first thing listed is referring to the cloud-datastore-admin app specifically. I went ahead and made a separate acl_decorators.

tjiang11 · 2018-09-15T20:53:43Z

Note that the current state does not properly work.

seanlip · 2018-09-16T06:13:41Z

export/cloud_datastore_admin.py

+        # Look for slash in the portion of the bucket URL that comes
+        # after 'gs://'. If not present, then only a bucket name has been
+        # provided and we append a trailing slash.
+        if '/' not in output_url_prefix[5:]:


You'll still want to do something about the "5:". Maybe extract 'gs://' into a named constant.

seanlip

Just a couple more things, otherwise LGTM. Thanks!

seanlip · 2018-09-17T02:34:22Z

export/app.yaml

+
+libraries:
+- name: webapp2
+  version: "latest"


Make this a specific version. (Don't want it to change under us.)

seanlip · 2018-09-17T02:34:49Z

export/cloud_datastore_admin.py

+import json
+import logging
+
+import oppia.export.acl_decorators as acl_decorators


from oppia.export import acl_decorators

tjiang11 added 10 commits September 8, 2018 23:24

"Update version number to 2.6.8"

5ba6541

Add files for export.

9731b2f

Lint.

07d640b

Lint.

60f20ba

Lint.

a564b8d

Lint.

059a313

Put export service into its own folder.

01eef2d

Moved cron job for export to base cron.yaml file.

0b3c2a8

lint.

a4c921f

Revert version.

6488bb2

seanlip requested changes Sep 12, 2018

View reviewed changes

tjiang11 added 2 commits September 15, 2018 16:01

Review changes.

cdde40c

Update cron.yaml, create init.py

296e1c2

seanlip reviewed Sep 16, 2018

View reviewed changes

tjiang11 added 4 commits September 16, 2018 21:31

Add acl_decorators for export service.

462b5ba

Lint.

20a76d9

Lint.

cb2f191

Remove magic number.

6550ba3

seanlip approved these changes Sep 17, 2018

View reviewed changes

Review changes.

3a13290

seanlip added the PR: LGTM label Sep 19, 2018

seanlip merged commit 701ea1e into develop Sep 21, 2018

seanlip deleted the export-schedule-dispatch branch September 21, 2018 02:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export schedule dispatch #5643

Export schedule dispatch #5643

tjiang11 commented Sep 12, 2018

codecov-io commented Sep 12, 2018 •

edited

Loading

seanlip left a comment

seanlip Sep 12, 2018

tjiang11 Sep 15, 2018

seanlip Sep 12, 2018

tjiang11 Sep 15, 2018

seanlip Sep 12, 2018

tjiang11 Sep 15, 2018

seanlip Sep 15, 2018 •

edited

Loading

tjiang11 Sep 15, 2018

seanlip Sep 12, 2018

tjiang11 Sep 15, 2018

seanlip Sep 12, 2018

tjiang11 Sep 15, 2018

seanlip Sep 15, 2018

tjiang11 Sep 16, 2018

seanlip Sep 16, 2018

seanlip Sep 12, 2018

tjiang11 Sep 15, 2018

seanlip Sep 12, 2018

tjiang11 Sep 15, 2018

seanlip Sep 16, 2018

tjiang11 Sep 17, 2018

tjiang11 commented Sep 15, 2018

seanlip Sep 16, 2018

tjiang11 Sep 17, 2018

seanlip left a comment

seanlip Sep 17, 2018

tjiang11 Sep 19, 2018

seanlip Sep 17, 2018

tjiang11 Sep 19, 2018

Export schedule dispatch #5643

Export schedule dispatch #5643

Conversation

tjiang11 commented Sep 12, 2018

codecov-io commented Sep 12, 2018 • edited Loading

Codecov Report

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjiang11 commented Sep 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 12, 2018 •

edited

Loading

seanlip Sep 15, 2018 •

edited

Loading