Skip to content
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

Push batch / transaction onto a thread-local stack inside 'with'. #518

Merged
merged 2 commits into from
Jan 8, 2015
Merged

Push batch / transaction onto a thread-local stack inside 'with'. #518

merged 2 commits into from
Jan 8, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jan 8, 2015

Prep. for #514.

@dhermes dhermes mentioned this pull request Jan 8, 2015
8 tasks
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e5e4518 on tseaver:push_batch_xact_on_stack_in_context_mgr into 77cfe10 on GoogleCloudPlatform:master.

if exc_type is None:
self.commit()
try:
if exc_type is None:

This comment was marked as spam.

Add explicit test of using the stack.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8190f4c on tseaver:push_batch_xact_on_stack_in_context_mgr into 77cfe10 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Jan 8, 2015

Nesting batches "works": it just "suspends" the outer batch until the inner one exits. It does mean that the "inner" changes will be sent to the back-end before the "outer" ones, but folks using Batch aren't supposed to care about that, right?

The problem I was trying to solve was to make discovery of any current batch / transaction straightforward, even if the application does weird stuff like nesting (maybe accidentally).

If nesting is not a desirable feature, then we can close this PR and do something like your suggestion instead.

@tseaver
Copy link
Contributor Author

tseaver commented Jan 8, 2015

Motivating example:

from gcloud.datastore.batch import _BATCHES, Batch

def put(*entities):
    current = _BATCHES.top
    in_batch = current is not None
    if not in_batch:
        current = Batch()
    for entity in entities:
        current.put(entity)
    if not in_batch:
        current.commit()

def delete(*keys):
    current = _BATCHES.top
    in_batch = current is not None
    if not in_batch:
        current = Batch()
    for key in keys:
        current.delete(key)
    if not in_batch:
        current.commit()

which will all Just Work(TM) even in the face of nesting.

@dhermes
Copy link
Contributor

dhermes commented Jan 8, 2015

I keep confusing myself. The key focus here is that with must be called and when __exit__ occurs the stack is happy. Hence there is no such thing as interleaving.

That said, I still think the maintenance cost of a stack is not worth the minimal gain in functionality (which many users may never see). Users motivated enough to want nested / conditional transactions can accomplish the same thing with the API provided byBatch. The complexity becomes their burden, not ours.

I'm torn between "it already works as intended" and "let's not add code that we don't need to".

@tseaver
Copy link
Contributor Author

tseaver commented Jan 8, 2015

That's actually why I made _BATCHES private (only capitalized as a sop to pylint :): Nobody but us chickens should touch it.

@dhermes
Copy link
Contributor

dhermes commented Jan 8, 2015

It's not us touching it that worries me, it's bugs from exotic usage in the wild that we don't / won't have bandwidth or desire to deal with.

For example, what if someone calls a method that uses a context manager under the hood, and they expect

with Transaction() as txn:
    funky_save()

to save in txn when instead it spawns a new transaction and doesn't save there.

As I said, I don't think the burden of a larger surface area justifies the small set of use cases that this enables syntactically.

I'm much more comfortable

  • Clearly documenting nesting is not allowed
  • Throwing an error if nesting is attempted

@tseaver
Copy link
Contributor Author

tseaver commented Jan 8, 2015

I don't see how your example is problematic: if funky_save manages its own transaction, separate from the surrounding one, their changes will still be saved, but just not "atomically" with txn's changes: that would be what they want, presumably (or they wouldn't code funky_save that way). We aren't doing damage by supporting nesting.

@silvolu Do the other mappings support nesting?

@dhermes
Copy link
Contributor

dhermes commented Jan 8, 2015

I'm not saying the functionality is damaging, I'm saying the maintenance burden on us is not worth it.

@tseaver
Copy link
Contributor Author

tseaver commented Jan 8, 2015

Hmm, the maintenance / explainability of the consistent / nested one seems easier to me than maintaining / explaining the "Highlander" case.

@dhermes
Copy link
Contributor

dhermes commented Jan 8, 2015

I suppose you're right. I'm probably just being paranoid. Let's hope it doesn't bite us.

@dhermes
Copy link
Contributor

dhermes commented Jan 8, 2015

LGTM

tseaver added a commit that referenced this pull request Jan 8, 2015
…ext_mgr

Push batch / transaction onto a thread-local stack inside 'with'.
@tseaver tseaver merged commit 7c01fcc into googleapis:master Jan 8, 2015
@tseaver tseaver deleted the push_batch_xact_on_stack_in_context_mgr branch January 8, 2015 22:14
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Jan 9, 2015
Also:
- Changing api.get() back to accept only `keys` and
  returns only a list (a lot of headache for not much
  gain).
- Factored out behavior to extract shared dataset_id from
  a set of keys into _get_dataset_id_from_keys().
- Updated docstrings and other tests that rely on changed /
  removed methods.

See googleapis#518 for some context.
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Jan 12, 2015
Also:
- Changing api.get() back to accept only `keys` and
  returns only a list (a lot of headache for not much
  gain).
- Factored out behavior to extract shared dataset_id from
  a set of keys into _get_dataset_id_from_keys().
- Updated docstrings and other tests that rely on changed /
  removed methods.

See googleapis#518 for some context.
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Jan 12, 2015
Also:
- Changing api.get() back to accept only `keys` and
  returns only a list (a lot of headache for not much
  gain).
- Factored out behavior to extract shared dataset_id from
  a set of keys into _get_dataset_id_from_keys().
- Updated docstrings and other tests that rely on changed /
  removed methods.

See googleapis#518 for some context.
@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Dec 31, 2015
parthea added a commit that referenced this pull request Aug 15, 2023
* chore: Prepare for mono repository migration

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
vchudnov-g pushed a commit that referenced this pull request Sep 20, 2023
Source-Link: googleapis/synthtool@453a5d9
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:81ed5ecdfc7cac5b699ba4537376f3563f6f04122c4ec9e735d3b3dc1d43dd32
parthea pushed a commit that referenced this pull request Sep 22, 2023
…p/templates/python_library/.kokoro (#518)

Source-Link: https://github.com/googleapis/synthtool/commit/bb171351c3946d3c3c32e60f5f18cee8c464ec51
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:f62c53736eccb0c4934a3ea9316e0d57696bb49c1a7c86c726e9bb8a2f87dadf
parthea pushed a commit that referenced this pull request Sep 22, 2023
Source-Link: https://github.com/googleapis/synthtool/commit/352b9d4c068ce7c05908172af128b294073bf53c
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:3e3800bb100af5d7f9e810d48212b37812c1856d20ffeafb99ebe66461b61fc7
parthea pushed a commit that referenced this pull request Oct 21, 2023
…pprove] (#518)

Source-Link: https://github.com/googleapis/synthtool/commit/e3a1277ac35fc88c09db1930533e24292b132ced
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:452901c74a22f9b9a3bd02bce780b8e8805c97270d424684bff809ce5be8c2a2
parthea added a commit that referenced this pull request Oct 21, 2023
Source-Link: googleapis/synthtool@7197a00
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:c43f1d918bcf817d337aa29ff833439494a158a0831508fda4ec75dc4c0d0320

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants