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.

Sign up for GitHub

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

Jump to bottom

Update 'Client' to hold stack of connection/batch/transaction objects. #976

Merged
merged 3 commits into from
Jul 10, 2015
Merged

Update 'Client' to hold stack of connection/batch/transaction objects. #976

merged 3 commits into from
Jul 10, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 9, 2015

See #944.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 9, 2015
@tseaver tseaver added the api: datastore Issues related to the Datastore API. label Jul 9, 2015
@dhermes
Copy link
Contributor

dhermes commented Jul 9, 2015

  1. The connection stack is meant to be like the _LocalStack? Can we give it a different name since it will presumably also hold transactions (and other non-connections).
  2. This stack isn't threadsafe. Is that relevant?
  3. Is the rest of the change coming soon?

@tseaver
Update docstrings
1d5ad10 
Objects pushe onto the client's '_connection_stack' must implement the
'Connection' API, but need not be subclasses of 'Connection'.

[ci skip]
@tseaver
Copy link
Contributor Author

tseaver commented Jul 9, 2015

The connection stack is meant to be like the _LocalStack? Can we give it a different name since it will presumably also hold transactions (and other non-connections).

They must all be implement the "connection" API. I can't think of a better name.

This stack isn't threadsafe. Is that relevant?

list.append and list.pop are atomic operations, which means that the list won't be corrupted by separate threads calling _push_connection. Threads using different batch / transaction objects with the same client could end up mangling one another: if it matters, we could just use the gcloud._helpers._LocalStack instead.

Is the rest of the change coming soon?

I think so. I threw out a much bigger changeset and started over, but I think this is simpler / clearer.

@tseaver tseaver mentioned this pull request Jul 9, 2015
Closed
8 tasks
@dhermes
Copy link
Contributor

dhermes commented Jul 9, 2015

But Transaction doesn't implement the same API as Connection. Am I missing something? Are there other things that will go on this stack?

Yeah the thread-safety issue is not resolved by this, can you file an issue so that we follow up? _LocalStack makes the stack local to only 1 thread?

@tseaver
Copy link
Contributor Author

tseaver commented Jul 10, 2015

@dhermes 82ee82c should address both your issues: it makes the stack hold only batch / transaction objects, and uses a _LocalStack for it (so that separate threads don't collide).

dhermes
dhermes reviewed Jul 10, 2015
View reviewed changes
gcloud/datastore/test_client.py
self.assertTrue(client.current_batch is None)
self.assertEqual(list(client._batch_stack), [])

def test__push_connection_and__pop_connection(self):

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Jul 10, 2015

LGTM other than small nit. I think the answer to my "is it expensive" question is "no", but maybe you can educate me.

@tseaver
Copy link
Contributor Author

tseaver commented Jul 10, 2015

I have a fix for the test name on the next PR.

ISTM that the expense of creating a thread-local object isn't really an issue for our Client, which will typically be constructed only at startup: the _BATCHES global is going away, so it will be net zero for the typical case.

tseaver added a commit that referenced this pull request Jul 10, 2015
@tseaver
Merge pull request #976 from tseaver/944-client_holds_connection_batc…
5148dc2 
…h_stack

Update 'Client' to hold stack of connection/batch/transaction objects.
@tseaver tseaver merged commit 5148dc2 into googleapis:master Jul 10, 2015
@tseaver tseaver deleted the 944-client_holds_connection_batch_stack branch July 10, 2015 17:08
@dhermes
Copy link
Contributor

dhermes commented Jul 10, 2015

Good call on net zero.

@dhermes dhermes mentioned this pull request Jul 10, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: datastore Issues related to the Datastore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tseaver @dhermes @googlebot