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

Move the 'thread-local stack' implementation up from datastore. #622

Merged
merged 3 commits into from
Feb 12, 2015
Merged

Move the 'thread-local stack' implementation up from datastore. #622

merged 3 commits into from
Feb 12, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Feb 12, 2015

Preparing to tackle #15, which needs similar support in storage.

Preparing to tackle #15, which needs similar support in storage.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 12, 2015
@tseaver tseaver added api: datastore Issues related to the Datastore API. api: core labels Feb 12, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 83ed437 on tseaver:move-thread_local_stack_up into 32e50eb on GoogleCloudPlatform:master.

# limitations under the License.
"""Thread-local resource stack.

Not an API.

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Feb 12, 2015

LGTM (I think you should clarify the module docstring, but do as you see fit).

@dhermes
Copy link
Contributor

dhermes commented Feb 12, 2015

@tseaver When merged can you ping me and point at the next PR in line?

@tseaver
Copy link
Contributor Author

tseaver commented Feb 12, 2015

I'm still working on the next-in-line (it is the real meat of #15).

tseaver added a commit that referenced this pull request Feb 12, 2015
Move the 'thread-local stack' implementation up from datastore.
@tseaver tseaver merged commit f73ad7e into googleapis:master Feb 12, 2015
@tseaver tseaver deleted the move-thread_local_stack_up branch February 12, 2015 21:43
@dhermes
Copy link
Contributor

dhermes commented Feb 12, 2015

OK cool

@tseaver
Copy link
Contributor Author

tseaver commented Feb 13, 2015

@dhermes I'm partway through the middle part here, if you are interested.

@dhermes
Copy link
Contributor

dhermes commented Feb 13, 2015

Just took a peek. I feel like the multipart stuff will have been handled already by @craigcitro

Also, what sorts of API requests are allowed with batch? Any and all?

@tseaver
Copy link
Contributor Author

tseaver commented Feb 14, 2015

I didn't see any of that in apitools. @craigcitro, can you comment? We are talking specifically about the batch request and batch response kinds of multipart API calls.

Batching can handle POST, PATCH, and DELETE for buckets, blobs, and their ACLS, but not GET (the batch object just proxies it through) nor the upload / download APIs (not yet excluded).

@craigcitro
Copy link
Contributor

writing from my phone - but apitools has full batch support. see base/py/batch.py

@tseaver
Copy link
Contributor Author

tseaver commented Feb 16, 2015

I didn't see the batching support in apitools because we didn't vendor in that module. Looking at it, I think we would be using the lower-level BatchHttpRequest class, because the higher-level BatchApiRequest.ApiCall class takes a service constructor paramter defined as being an instance of a subclass of apitools.base.py.base_api.BaseApiServie, which would put us back in protorpc land.

@craigcitro It looks like the BatchHttpRequest does not preserve the original ordering of the batched requests (they are stored in a dict, whose keys are iterated when building the payload. I'm not sure if that would matter, but it bugs me a bit.

@tseaver
Copy link
Contributor Author

tseaver commented Feb 16, 2015

apitools/base/py/batch.py module doesn't appear to have test coverage, and does not yet straddle Py3k.

@craigcitro
Copy link
Contributor

@tseaver ah, i probably don't export batch_test.py from internal. trying to get that sync'd up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core api: datastore Issues related to the Datastore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants