-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Transaction propagation using ndb.TransactionOptions #537
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Apologies for the long delay. I'm reaching out to the Datastore backend team for details on this feature and will be back with some more info as soon as that is resolved. |
Is there any update on this that you can share with me? Please do let me know if I can be of assistance in any way. |
@JohnGale87 Thanks again for your patience. We were initially concerned that this behavior should be tying into server-side functionality we were missing, but we've looked into it and it looks like it should be here in the client after all. Thanks for including tests as well! We'll review this PR for inclusion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please take a look at the comments. This is almost ready to merge.
@JohnGale87 I ran the tests again and noticed that in addition to Python 2.7 compatibility, this PR needs to get to 100% unit test coverage and pass the lint checks (run nox -e blacken, then nox -e lint). Thanks for all your effort. |
@JohnGale87 Thanks for your recent changes! Getting closer, lint and python 2.7 compatibility are passing now. The test coverage is still not 100% however. Could you please work in a few more tests (run nox -e cover for info)? Also, a docstring for the class would be great. Thanks! |
@cguardia Yes sorry, I'm in the habit of pushing every time I commit, didn't realise it would be spamming you. I am still working on it. I'll try to stop pushing to git until I'm finished! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See notes.
…e additional arguments for the new context. remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, thank you! Just a couple of editing notes on the docs.
spelling and punctuation for docstrings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Hi Cloud NDB team,
I did review your CONTRIBUTING.rst but if I've done anything wrong then please let me know and I will of course endevour to correct it. I've never done this before so wasn't sure of the process - do I just submit a pull request straight away or am I supposed to open an issue first...?
Anyway, our application uses the existing
google.appengine.ext.ndb
library and in doing so makes relatively extensive use of the propagation options fromndb.TransactionOptions
. So knowing that you marked them all asNoLongerImplementedError
has been a bit of a blocker for us migrating off of python 2.A few months ago I followed closely on #366 and that also lead me to read #4.
Having now read those issues and from what I understand of the discussion(s) they contained combined with my observations in both the old and new implementations, the main reason I can discern for the new library not including the original propagation options seems mostly to do with the
ndb.TransactionOptions.NESTED
option. Which the documentation says isn't actually supported by the old AppEngine python ndb library anyway...In #381 you added the
join
parameter and for@ndb.transactional
set the default to beTrue
which seems to me to be the equivalent ofndb.TransactionOptions.ALLOWED
being the default propagation option in the old library (please correct me if I'm wrong).Based on those assumptions I have been looking at the feasibility of adding equivalents of
ndb.TransactionOptions.MANDATORY
andndb.TransactionOptions.INDEPENDENT
. I think I have a solution for doing so (presented in this pull request) but wanted to have my thoughts validated and my code reviewed and (likely) refined.My implementation currently maintains compatibility with the
join
argument (I think) so that any users who have already started to use that will not be impacted by these changes. Although further changes would make it possible for the propagation argument to supersede and replace thejoin
argument if wanted.The implementation I am most unsure about is the
ndb.TransactionOptions.INDEPENDENT
one. I used a similar transaction pausing technique as was used innon_transactional
but in the newly created context I set some additional arguments to None in an effort to ensure that the new transaction shares nothing with the previous one. Admittedly I'm not 100% sure if I should have done that or not, or if I got all the right arguments, but I'm hoping your review will reveal the answer to that.