initial version of migration guide #121
Conversation
googlebot
added
the
cla: yes
docs/migrating.rst
Outdated
| available. It still uses the same signature, but does not support original | ||
| Configuration methods. | ||
|
|
||
| Similarly,b ecause ``google.appengine.datastore.datastore_query.Order`` is no |
There was a problem hiding this comment.
Typo: s/Similarly,b ecause/Similarly, because/
| ==== | ||
|
|
||
| There are some methods from the ``key`` module that are not implemented in | ||
| this version of `ndb`: |
There was a problem hiding this comment.
Do we want to explain why?
There was a problem hiding this comment.
I agree. Based on https://cloud.google.com/appengine/docs/standard/python/ndb/keyclass#class_methods, I think this could say:
These methods were used to pass keys to and from the db Datastore API, which is no longer supported.
docs/migrating.rst
Outdated
| - The `BlobProperty` constructor only sets `_compressed` if explicitly | ||
| passed. The original set `_compressed` always. | ||
| - In the exact same fashion the `JsonProperty` constructor only sets | ||
| `_json_type` if explicitly passed.] |
docs/migrating.rst
Outdated
|
|
||
| client = ndb.Client() | ||
|
|
||
| with context as client.context(): |
There was a problem hiding this comment.
swap context and client.context()
docs/migrating.rst
Outdated
|
|
||
| Aside from this, there are many differences between the Datastore APIs | ||
| provided by GAE and those provided by the newer Google Cloud Platform. These | ||
| diffeences have required some code and API changes as well. |
docs/migrating.rst
Outdated
| - ReducedFuture. | ||
| - SerialQueueFuture. | ||
| - set_context. | ||
| - toplevel. |
There was a problem hiding this comment.
I think we can still implement toplevel. Or did you look at it and find out we couldn't?
There was a problem hiding this comment.
I only took a quick glance while going through the TODO list, and I thought its reliance on _make_cloud_datastore_context meant it was out. I didn't stop to think about the implementation. Do you think I should remove it from this list?
There was a problem hiding this comment.
I think it's doable. Let's keep this on our TODO list, still.
| made obsolete by new Python 3 features, while others have been discarded due | ||
| to implementation differences in the new `ndb`. | ||
|
|
||
| Possibly the most used utility from this module outside of `ndb` code, is the |
There was a problem hiding this comment.
Until we go back and add py27 compatibility, at least.
| because that's the namespace passed in when setting up the client. | ||
|
|
||
| Django Middleware | ||
| ================= |
There was a problem hiding this comment.
So, a couple things here:
-
A context is thread local. Recommended practice is one context per request. So you probably want to create the client in
__init__but create the context in__call__. -
__call__looks like a great place to use awithstatement.
Maybe something more like:
from google.cloud import ndb
class NDBMiddleware(object):
def __init__(self, get_response):
self.get_response = get_response
self.client = ndb.Client()
def __call__(self, request):
context = self.client.context()
request.ndb_context = context
with context:
response = self.get_response(request)
return response
There was a problem hiding this comment.
If it is any help, putting this in my Django middleware, I am receiving AttributeError: '_GeneratorContextManager' object has no attribute 'use'. Seems to be working without the use but I don't have enough "context" to know if that will break something else.
Putting the context in the __init__ definitely broke things for the second request that uses ndb.
There was a problem hiding this comment.
Thanks a lot, I'm going to test this more and make it work before the merge.
There was a problem hiding this comment.
Right, I forgot that Client.context calls Context.use for you. I will correct my snippet.
docs/migrating.rst
Outdated
| shims for most of them, which can be imported from the `ndb.exceptions` | ||
| package, like this:: | ||
|
|
||
| from ndb.exceptioms import BadRequestError, BadArgumentError |
docs/migrating.rst
Outdated
| implementation, and sometimes even to remove exisitng functionality | ||
| altogether. | ||
|
|
||
| Because one of the main objectives of this rewrite was to be able to use `ndb` |
There was a problem hiding this comment.
I initially assumed "independently from GAE" meant "in another Google Cloud Product". How about:
One of the main objectives of this rewrite was to enable ndb for use in any Python environment, not just Google App Engine. As a result, many of the ndb APIs that relied on GAE environment and runtime variables, resources, and legacy APIs have been dropped.
| ==== | ||
|
|
||
| There are some methods from the ``key`` module that are not implemented in | ||
| this version of `ndb`: |
There was a problem hiding this comment.
I agree. Based on https://cloud.google.com/appengine/docs/standard/python/ndb/keyclass#class_methods, I think this could say:
These methods were used to pass keys to and from the db Datastore API, which is no longer supported.
docs/migrating.rst
Outdated
| implementation, and sometimes even to remove exisitng functionality | ||
| altogether. | ||
|
|
||
| Because one of the main objectives of this rewrite was to be able to use `ndb` |
There was a problem hiding this comment.
I assumed "use ndb independently from Google App Engine" meant "use nbd in another Google Cloud product." How about changing the paragraph to this:
One of the main objectives of this rewrite was to enable ndb for use in any Python environment, not just Google App Engine. As a result, ndb APIs that depended on GAE environment and runtime variables, resources, or legacy APIs have been dropped.
docs/migrating.rst
Outdated
| - Similarly, the `DateTimeProperty` constructor only sets `_auto_now` and | ||
| `_auto_now_add` if explicitly passed. | ||
| - `TextProperty(indexed=True)` and `StringProperty(indexed=False)` are no | ||
| longer supported. |
There was a problem hiding this comment.
Does this mean that TextProperty can no longer be indexed, whereas StringProperty is always indexed?
If so, for clarification can you add:
That is, TextProperty can no longer be indexed, whereas StringProperty is always indexed.
docs/migrating.rst
Outdated
| significantly from the public API used by App Engine to the current published | ||
| API. Additionally, this version of NDB mostly delegates to | ||
| `google.cloud.datastore` for parsing data returned by RPCs, which is a | ||
| significant internal refactoring. |
There was a problem hiding this comment.
I'm a little confused here. Are you saying that the old NDB library included some undocumented APIs that dealt directly with protocol buffers? Also, from reading the MIGRATION_NOTES, I'm not sure it would be possible for someone to tell which APIs dealt with protobufs.
How about this:
The old NDB library included some undocumented APIs that dealt directly with Datastore protocol buffers. These APIs will no longer work. Rewrite any code that used the following classes, properties, or methods:
- ModelAdapter
- Property._db_get_value, Property._db_set_value
- Property._db_set_compressed_meaning and Property._db_set_uncompressed_meaning
- Model._deserialize and Model._serialize
- model.make_connection
|
@chrisrossi I think this is ready for another pass. |
|
@darrencarlton @lolsheeplol @BenWhitehead @chrisrossi @sadovnychyi @plumSemPy Thanks for all your input and suggestions. We are looking into how/where to best incorporate this documentation into the project. |
docs/migrating.rst
Outdated
| - Key.from_old_key. | ||
| - Key.to_old_key. | ||
|
|
||
| These methods were used to pass keys to and from the db Datastore API, which is |
There was a problem hiding this comment.
Should make obvious that db is a name of an old product. In this case, the predecessor to ndb.
docs/migrating.rst
Outdated
| - ReducedFuture. | ||
| - SerialQueueFuture. | ||
| - set_context. | ||
| - toplevel. |
There was a problem hiding this comment.
I think it's doable. Let's keep this on our TODO list, still.
docs/migrating.rst
Outdated
| - Model._deserialize and Model._serialize. | ||
| - model.make_connection. | ||
|
|
||
| will no longer work. The Datastore `.protobuf` definitions have changed |
There was a problem hiding this comment.
This needs to be reworked or elided following the edit above.
cguardia
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
docs/migrating.rst
Outdated
| we can no longer assume it's running in the context of a GAE request. | ||
|
|
||
| The `ndb` client uses ``google.auth`` for authentication, which is how APIs in | ||
| Google Cloud Platform work. The client can take a `credentials` parameter or |
There was a problem hiding this comment.
Recommend "The ndb client uses google.auth for authentication, consistent with other Google Cloud Platform client libraries."
docs/migrating.rst
Outdated
| The `ndb` client uses ``google.auth`` for authentication, which is how APIs in | ||
| Google Cloud Platform work. The client can take a `credentials` parameter or | ||
| get the credentials using the `GOOGLE_APPLCATION_CREDENTIALS` environment | ||
| variable, which is the recommended option. |
There was a problem hiding this comment.
Recommend we also link to https://cloud.google.com/storage/docs/reference/libraries for authentication advice.
docs/migrating.rst
Outdated
| The context is not thread safe, so for threaded applications, you need to | ||
| generate one context per thread. This is particularly important for web | ||
| applications, where the best practice would be to generate a context per | ||
| request. |
There was a problem hiding this comment.
This is confusing. If you have a web application, where the best practice would be to generate one context per request, and you need multiple threads inside that request, what should you do?
If the answer is "generate multiple contexts" then maybe something like this?
"In a web application, a typical best practice is to generate one context per request. However, a context object only applies to a single thread; in the case that multiple threads are used for a single request, a new context should be generated for every thread that will use the NDB library."
This required a good deal of research. I tried to document why we are not implementing some stuff from what's left in the TODO document. Please take a look to see if I'm wrong on something.