Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entities' #429

tseaver · 2014-12-17T19:27:33Z

If 'deferred' list is not passed, the connection retries any keys in
a deferred response.

Closes #306.

…ies'. If 'deferred' list is not passed, the connection retries any keys in a deferred response. Closes #306.

gcloud/datastore/connection.py


-        results = [result.entity for result in lookup_response.found]
+            # Hmm, should we sleep here?  Asked in:
+            # https://github.com/GoogleCloudPlatform/gcloud-python/issues/306#issuecomment-67377587


coveralls · 2014-12-17T19:32:11Z

Coverage remained the same when pulling 6b1b431 on tseaver:306-connection_lookup_missing_deferred into ac999e8 on GoogleCloudPlatform:master.

gcloud/datastore/connection.py

+                break
+
+            if lookup_response.deferred:  # retry
+                for old_key in list(lookup_request.key):


dhermes · 2014-12-17T19:54:22Z

Did a full review, everything looks good pending comments.

Also don't forget to remove the question in the comment about the sleep.

@pcostell

@pcostell says not to bother.

@dhermes

Addresses @dhermes comment: https://github.com/GoogleCloudPlatform/gcloud-python/pull/429/files#r21997643

@dhermes

Addresses @dhermes comments: https://github.com/GoogleCloudPlatform/gcloud-python/pull/429/files#r21998236 https://github.com/GoogleCloudPlatform/gcloud-python/pull/429/files#r21998714

coveralls · 2014-12-17T20:23:48Z

Coverage remained the same when pulling 2db4b5d on tseaver:306-connection_lookup_missing_deferred into ac999e8 on GoogleCloudPlatform:master.

dhermes · 2014-12-17T20:26:04Z

So we still need to address [:] vs. .extend issue and the question of looping for a massive number of entities in a fetch.

coveralls · 2014-12-17T20:36:40Z

Coverage remained the same when pulling 5ab96e8 on tseaver:306-connection_lookup_missing_deferred into ac999e8 on GoogleCloudPlatform:master.

tseaver · 2014-12-17T20:39:22Z

@dhermes if the back-end imposed a limit on number of keys, we could just let that error propagate.

As it is, if the user straps a cannon to his shin and pulls the trigger, we can't do much to help. They may get a 503 (for DoSing the API) anyway, and if not, they eventually get their data (even if they rack up big charges for it).

dhermes · 2014-12-17T20:43:57Z

We can make it harder to do pathological things, like putting the cannon in a locked cabinet in your analogy.

From previous discussions with @pcostell, the backend limits the number of keys and also the length of the request, but after that just dumps them in deferred. Now that you mention it though, 100,000 keys would probably exceed a maximum payload size of a request, which I'm sure exists.

dhermes · 2014-12-17T21:01:57Z

Eek, lint failure.

@tseaver to make tox -e lint faster locally, I use

export GCLOUD_REMOTE_FOR_LINT="official"  # You use upstream I believe
export GCLOUD_BRANCH_FOR_LINT="master"

I wrote about it in the CONTRIBUTING.md file but I'm not sure anyone other than me uses it.

tseaver · 2014-12-17T21:08:17Z

I normally run 'tox' (all environments). 9cd0f6a will clean it up.

tseaver · 2014-12-17T21:09:08Z

Note also that running with those variables sometimes leads to false-positives (weird cases where the not-included files somehow help to satisfy a check).

coveralls · 2014-12-17T21:11:34Z

Coverage remained the same when pulling 9cd0f6a on tseaver:306-connection_lookup_missing_deferred into ac999e8 on GoogleCloudPlatform:master.

tseaver · 2014-12-17T21:17:12Z

@dhermes anything left unaddressed?

Note that this kind of review is where the Github UI review breaks down: it is too hard for either the contributor or the reviewer to figure out what issues remain to be resolved.

dhermes · 2014-12-17T21:30:11Z

The only remaining thing is the question about protecting novice users from themselves, which we can punt on in an issue.

Hopefully @pcostell or someone else can chime in on the discussion in #306.

LGTM

…ferred Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entities'

…429) Source-Link: https://github.com/googleapis/synthtool/commit/b4fe62efb5114b6738ad4b13d6f654f2bf4b7cc0 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:3bf87e47c2173d7eed42714589dc4da2c07c3268610f1e47f8e1a30decbfc7f1

- [ ] Regenerate this pull request now. PiperOrigin-RevId: 473833416 Source-Link: googleapis/googleapis@565a550 Source-Link: googleapis/googleapis-gen@1ee1a06 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMWVlMWEwNmM2ZGUzY2E4Yjg0MzU3MmMxZmRlMDU0OGY4NDIzNjk4OSJ9

* docs: Fix formatting of request arg in docstring chore: Update gapic-generator-python to v1.9.1 PiperOrigin-RevId: 518604533 Source-Link: googleapis/googleapis@8a085ae Source-Link: googleapis/googleapis-gen@b2ab4b0 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYjJhYjRiMGEwYWUyOTA3ZTgxMmMyMDkxOThhNzRlMDg5OGFmY2IwNCJ9 * 🦉 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>

* chore(deps): update all dependencies * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * revert Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Anthonios Partheniou <partheniou@google.com>

…429) Source-Link: https://github.com/googleapis/synthtool/commit/395d53adeeacfca00b73abf197f65f3c17c8f1e9 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:6c1cbc75c74b8bdd71dada2fa1677e9d6d78a889e9a70ee75b93d1d0543f96e1

- [ ] Regenerate this pull request now. docs: list oneofs in docstring docs(v2beta1): clarified meaning of the legacy editions docs(v2beta1): clarified semantic of the streaming APIs fix(deps): require google-api-core >= 1.28.0 fix(deps): drop packaging dependency committer: busunkim96@ PiperOrigin-RevId: 406468269 Source-Link: googleapis/googleapis@83d81b0 Source-Link: googleapis/googleapis-gen@2ff001f Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMmZmMDAxZmJhY2I5ZTc3ZTcxZDczNGRlNWY5NTVjMDVmZGFlODUyNiJ9

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

Co-authored-by: Anthonios Partheniou <partheniou@google.com>

Source-Link: https://github.com/googleapis/synthtool/commit/0ddbff8012e47cde4462fe3f9feab01fbc4cdfd6 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:bced5ca77c4dda0fd2f5d845d4035fc3c5d3d6b81f245246a36aee114970082b

…[autoapprove] (#429) Source-Link: googleapis/synthtool@1f37ce7 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:8e84e0e0d71a0d681668461bba02c9e1394c785f31a10ae3470660235b673086 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entit…

6b1b431

…ies'. If 'deferred' list is not passed, the connection retries any keys in a deferred response. Closes #306.

tseaver reviewed Dec 17, 2014
View reviewed changes

gcloud/datastore/connection.py

results = [result.entity for result in lookup_response.found]

# Hmm, should we sleep here? Asked in:

# https://github.com/GoogleCloudPlatform/gcloud-python/issues/306#issuecomment-67377587

This comment was marked as spam.

Sign in to view

dhermes reviewed Dec 17, 2014
View reviewed changes

gcloud/datastore/connection.py

break

if lookup_response.deferred: # retry

for old_key in list(lookup_request.key):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

tseaver added 3 commits December 17, 2014 14:59

Remove note about sleep.

45cdac1

@pcostell says not to bother.

Move copy-deferred-keys-to-request logic out of 'Connection.lookup'.

b9db057

Addresses @dhermes comment: https://github.com/GoogleCloudPlatform/gcloud-python/pull/429/files#r21997643

Squash checks of protobuf call headers into helper method.

2db4b5d

Addresses @dhermes comments: https://github.com/GoogleCloudPlatform/gcloud-python/pull/429/files#r21998236 https://github.com/GoogleCloudPlatform/gcloud-python/pull/429/files#r21998714

Document that / lists passed as arguments must be empty.

5ab96e8

Enforce that / lists passed as arguments must be empty.

f11fa24

Dead chickens on the altar of pylint/pep8 continuation taboos.

9cd0f6a

tseaver added a commit that referenced this pull request Dec 17, 2014

Merge pull request #429 from tseaver/306-connection_lookup_missing_de…

c2ba130

…ferred Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entities'

tseaver merged commit c2ba130 into googleapis:master Dec 17, 2014

tseaver deleted the 306-connection_lookup_missing_deferred branch December 17, 2014 21:54

dhermes added the api: datastore Issues related to the Datastore API. label Dec 31, 2015

parthea pushed a commit that referenced this pull request Jun 4, 2023

chore(deps): update dependency google-cloud-bigquery to v3.4.2 (#429)

4acd366

parthea pushed a commit that referenced this pull request Sep 22, 2023

chore(main): release 3.10.1 (#429)

01863ec

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

parthea added a commit that referenced this pull request Sep 22, 2023

chore(deps): update all dependencies (#429)

8ad2a1b

Co-authored-by: Anthonios Partheniou <partheniou@google.com>

parthea pushed a commit that referenced this pull request Oct 21, 2023

chore(deps): update all dependencies (#429)

e0a95af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entities' #429

Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entities' #429

tseaver commented Dec 17, 2014

This comment was marked as spam.

coveralls commented Dec 17, 2014

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Dec 17, 2014

coveralls commented Dec 17, 2014

dhermes commented Dec 17, 2014

coveralls commented Dec 17, 2014

tseaver commented Dec 17, 2014

dhermes commented Dec 17, 2014

dhermes commented Dec 17, 2014

tseaver commented Dec 17, 2014

tseaver commented Dec 17, 2014

coveralls commented Dec 17, 2014

tseaver commented Dec 17, 2014

dhermes commented Dec 17, 2014

Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entities' #429

Expose 'missing'/'deferred' in 'Connection.lookup'/'Dataset.get_entities' #429

Conversation

tseaver commented Dec 17, 2014

This comment was marked as spam.

coveralls commented Dec 17, 2014

This comment was marked as spam.

This comment was marked as spam.

dhermes commented Dec 17, 2014

coveralls commented Dec 17, 2014

dhermes commented Dec 17, 2014

coveralls commented Dec 17, 2014

tseaver commented Dec 17, 2014

dhermes commented Dec 17, 2014

dhermes commented Dec 17, 2014

tseaver commented Dec 17, 2014

tseaver commented Dec 17, 2014

coveralls commented Dec 17, 2014

tseaver commented Dec 17, 2014

dhermes commented Dec 17, 2014