Replace App Engine NDB with google cloud NDB. #1298

oliverchang · 2019-12-19T05:23:27Z

Part of migration to Python 3. This is our last major blocker and
dependency on the Python 2 App Engine SDK.

Also remove ndb_patcher, as that is no longer required.

Notable changes:

indexed=False is no longer supported for StringProperty. Use TextProperty instead, which is
equivalent.
batch_size query keyword arg is no longer supported (Datastore backend change).
fetch_page() is no longer supported for complex queries with !=, OR etc.
Many import hacks/workarounds to make things work for App Engine.
- Notable: add grpcio to app.yaml, cron-service.yaml
Add workaround for cursor_after() doesn't exist at end of iteration googleapis/python-ndb#292 and Can no longer pass property types to projection argument ? googleapis/python-ndb#295
Threads may be created by the new NDB implementation, which are only guaranteed to be
stopped after a GC. GAE doesn't like it when there are stray threads at the end of a request.
Stub out ndb_utils.get_all_from_query. This is no longer required, and callers will be cleaned up in a future PR.
All datastore operations now require:

from datastore import ndb_init
with ndb_init.context():
  ...

This is automatically set up in tests, as well as in run_bot, run, heartbeat, run_heartbeat, and butler run, as well as for each individual request.

oliverchang · 2020-01-15T04:34:17Z

One more blocker: googleapis/python-ndb#292

oliverchang · 2020-01-20T04:35:34Z

/gcbrun

oliverchang · 2020-01-20T05:50:03Z

Update: I've now gotten this to run on real GAE, but now the testcases list page takes literally minutes to load.

oliverchang · 2020-01-21T12:58:34Z

Went down a pretty deep rabbit hole to figure out why this testcases list (and only this page) was hanging with this new NDB.

In case anyone is interested, the root cause is that App Engine doesn't like it when a request creates threads which are still running at the end -- this will appear as if the request never completes, without any obvious indication why. This was also only reproducible on GAE, and not local dev_appserver.

The new NDB uses gRPC, which in turn uses google.auth.transports.grpc, which creates a ThreadPoolExecutor with 1 thread.

Somehow, when a MultiQuery happens (in the case of testcases list), things are set up such that there are references cycles which prevent the threads from getting stopped normally here.

So, at the end of every request, we explicitly call the GC, which fixes this.

There is also one more bug in NDB that I filed: googleapis/python-ndb#295

oliverchang · 2020-01-21T13:09:56Z

/gcbrun

oliverchang · 2020-01-22T02:38:17Z

/gcbrun

oliverchang · 2020-01-22T03:27:37Z

/gcbrun

oliverchang · 2020-01-22T03:55:59Z

/gcbrun

oliverchang · 2020-01-28T00:40:33Z

Don't see any exceptions on the bots I staged over the weekend. This is ready for review while I do some more testing.

oliverchang · 2020-01-28T00:41:04Z

(CI will fail right now due to a conflicting dependency in the image, which is removed by this PR).

mbarbella-chromium · 2020-01-28T00:52:01Z

src/appengine/libs/query/datastore_query.py

+
+  next = __next__
+
+  def last_cursor(self):


Nit: docstring

mbarbella-chromium · 2020-01-28T00:58:47Z

src/python/datastore/data_types.py

-  fuzzed_keys = StringProperty(indexed=False)
-  minimized_keys = StringProperty(indexed=False)
-  minidump_keys = StringProperty(indexed=False)
+  fuzzed_keys = TextProperty()


Do you know how these are stored after we put the entities? I'm curious if these would be stringValues, blobValues, or something else.

These are always stored as strings (assuming utf-8 encoding if byte are passed), per https://github.com/googleapis/python-ndb/blob/662a88a82691e69d282565b9d11706237da6f654/google/cloud/ndb/model.py#L2698

Also validated this by running the code.

inferno-chromium

Super awesome!, lets check this in to avoid b/148362873

inferno-chromium · 2020-01-28T20:47:10Z

src/appengine/handlers/crash_stats.py

@@ -131,11 +131,7 @@ def query_testcase(project_name, crash_type, crash_state, security_flag,
      data_types.Testcase.open == is_open,
      ndb_utils.is_false(data_types.Testcase.is_a_duplicate_flag)).order(
          -data_types.Testcase.timestamp).iter(
-              limit=1,
-              projection=[
-                  data_types.Testcase.bug_information,


Is this really needed ? i prefer this style reference via doing typos in strings.

This is googleapis/python-ndb#292.

Sorry meant googleapis/python-ndb#295

inferno-chromium · 2020-01-28T20:48:13Z

src/appengine/libs/query/datastore_query.py

@@ -53,12 +55,42 @@ def _combine(q1, q2):
  return result


+# TODO(ochang): Remove once bug is fixed.


link to bug

inferno-chromium · 2020-01-28T20:49:44Z

src/appengine/libs/query/datastore_query.py

+
+    return next(self._it)
+
+  next = __next__


nite: move to start of class (not in middle of functions).

This is a common pattern (see e.g. https://github.com/googleapis/python-ndb/blob/4ffedc7b5a2366be15dcd299052d8a46a748addd/google/cloud/ndb/_datastore_query.py#L468). Putting it before won't work either becausess next won't be defined at that point. This is just to create an alias for python2/3 compatibility.

inferno-chromium · 2020-01-28T20:52:52Z

src/python/datastore/data_types.py

@@ -385,7 +385,7 @@ class Testcase(Model):
  crash_type = StringProperty()

  # Crashing address.
-  crash_address = StringProperty(indexed=False)
+  crash_address = TextProperty()


Will we lose values in old Testcase entities. Many times i have seen this if we change data type, it resets values in old entities ?

Nope. TextProperty is equivalent to StringProperty(indexed=False) (but the new NDB requires you to just use TextProperty instead).

inferno-chromium · 2020-01-28T20:53:29Z

src/python/datastore/data_types.py

@@ -631,6 +631,10 @@ def _pre_put_hook(self):
    self.populate_indices()

  def _post_put_hook(self, _):
+    if not self.key:
+      # Failed put.
+      return


Do we need to log ? How is this expected, maybe expand comment.

This was always wrong -- _post_put_hook can be called even if there was an exception in put(), and the exception will be rethrown automatically afterwards. Expanded the comment a bit more.

DO NOT MERGE YET. Part of migration to Python 3. This is our last major blocker and dependency on the Python 2 App Engine SDK. Also remove ndb_patcher, as that is no longer required.

This reverts commit 2153069.

oliverchang · 2020-01-29T00:03:02Z

/gcbrun

oliverchang · 2020-01-29T00:03:23Z

/gcbrun

oliverchang · 2020-01-29T00:15:24Z

/gcbrun

oliverchang · 2020-01-29T01:24:56Z

/gcbrun

oliverchang · 2020-01-29T02:14:49Z

/gcbrun

oliverchang · 2020-01-29T02:36:33Z

phew, CI is finally passing. Merging!

This reverts commit 0db3d6c.

googlebot added the cla: yes CLA signed. label Dec 19, 2019

oliverchang force-pushed the ndb branch from 4cfb086 to aa7bd06 Compare January 15, 2020 04:26

oliverchang force-pushed the ndb branch from aa7bd06 to 9ab1722 Compare January 20, 2020 04:30

oliverchang force-pushed the ndb branch from 943e69d to 3145408 Compare January 21, 2020 13:09

oliverchang force-pushed the ndb branch from 3145408 to 6e519ee Compare January 22, 2020 01:41

oliverchang force-pushed the ndb branch 2 times, most recently from f0dbc8b to 88a3217 Compare January 24, 2020 06:53

oliverchang requested review from mbarbella-chromium and inferno-chromium January 28, 2020 00:40

oliverchang changed the title ~~(DON'T MERGE YET) Replace App Engine NDB with google cloud NDB.~~ Replace App Engine NDB with google cloud NDB. Jan 28, 2020

mbarbella-chromium approved these changes Jan 28, 2020

View reviewed changes

oliverchang force-pushed the ndb branch from 88a3217 to e465ea2 Compare January 28, 2020 04:00

inferno-chromium approved these changes Jan 28, 2020

View reviewed changes

oliverchang added 7 commits January 29, 2020 10:11

Replace App Engine NDB with google cloud NDB.

077d5af

DO NOT MERGE YET. Part of migration to Python 3. This is our last major blocker and dependency on the Python 2 App Engine SDK. Also remove ndb_patcher, as that is no longer required.

heartbeat

0caa556

fixes

b0b0114

workaround bug

921a6d4

more fixes

6409242

remove unnecessary code

879c272

another init

9a56d7e

oliverchang added 14 commits January 29, 2020 10:11

fix appengine pkg_resources error

b8c55cf

fix projection

4d11a81

gc at end of every request

f37bf2d

fix tests

d6180d4

fix projection args and get_all_from_query

321ed04

fix lint

08c3756

fix exception

8738f05

fix rebase

4b9b1a4

client project

7d73959

fix put hook

7ff0ad4

temp

13d7982

Revert "Remove hacks"

d4946b7

This reverts commit 2153069.

Add docstring

8706853

address comments

4c31c17

oliverchang force-pushed the ndb branch from e465ea2 to 4c31c17 Compare January 29, 2020 00:03

fix imports from recent new code.

aa7d991

add more workarounds

fd9dbe1

alternate workaround

eedc11b

oliverchang merged commit 0db3d6c into master Jan 29, 2020

oliverchang added a commit that referenced this pull request Jan 29, 2020

Revert "Replace App Engine NDB with google cloud NDB. (#1298)"

6501c4d

This reverts commit 0db3d6c.

oliverchang mentioned this pull request Jan 29, 2020

Revert "Replace App Engine NDB with google cloud NDB." #1389

Merged

oliverchang added a commit that referenced this pull request Jan 29, 2020

Revert "Replace App Engine NDB with google cloud NDB. (#1298)" (#1389)

11650b8

This reverts commit 0db3d6c.

oliverchang deleted the ndb branch January 30, 2020 03:06

oliverchang mentioned this pull request Feb 20, 2020

[GAE python 2.7] query().filter().fetch() sometimes spawns threads preventing webapp2 requests finish within a deadline googleapis/python-ndb#336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace App Engine NDB with google cloud NDB. #1298

Replace App Engine NDB with google cloud NDB. #1298

oliverchang commented Dec 19, 2019 •

edited

Loading

oliverchang commented Jan 15, 2020

oliverchang commented Jan 20, 2020

oliverchang commented Jan 20, 2020 •

edited

Loading

oliverchang commented Jan 21, 2020 •

edited

Loading

oliverchang commented Jan 21, 2020

oliverchang commented Jan 22, 2020

oliverchang commented Jan 22, 2020

oliverchang commented Jan 22, 2020

oliverchang commented Jan 28, 2020

oliverchang commented Jan 28, 2020

mbarbella-chromium Jan 28, 2020

oliverchang Jan 28, 2020

mbarbella-chromium Jan 28, 2020

oliverchang Jan 28, 2020

inferno-chromium left a comment

inferno-chromium Jan 28, 2020

oliverchang Jan 28, 2020

oliverchang Jan 29, 2020

inferno-chromium Jan 28, 2020

oliverchang Jan 28, 2020

inferno-chromium Jan 28, 2020

oliverchang Jan 28, 2020

inferno-chromium Jan 28, 2020

oliverchang Jan 28, 2020

inferno-chromium Jan 28, 2020

oliverchang Jan 28, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

		@@ -53,12 +55,42 @@ def _combine(q1, q2):
		return result


		# TODO(ochang): Remove once bug is fixed.

Replace App Engine NDB with google cloud NDB. #1298

Replace App Engine NDB with google cloud NDB. #1298

Conversation

oliverchang commented Dec 19, 2019 • edited Loading

oliverchang commented Jan 15, 2020

oliverchang commented Jan 20, 2020

oliverchang commented Jan 20, 2020 • edited Loading

oliverchang commented Jan 21, 2020 • edited Loading

oliverchang commented Jan 21, 2020

oliverchang commented Jan 22, 2020

oliverchang commented Jan 22, 2020

oliverchang commented Jan 22, 2020

oliverchang commented Jan 28, 2020

oliverchang commented Jan 28, 2020

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

inferno-chromium 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

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

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Jan 29, 2020

oliverchang commented Dec 19, 2019 •

edited

Loading

oliverchang commented Jan 20, 2020 •

edited

Loading

oliverchang commented Jan 21, 2020 •

edited

Loading