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

Datastore: add retry param to page iterator. #8547

Conversation

pchauhan-qlogic
Copy link
Contributor

Fixes #6119

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 1, 2019
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 4, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 4, 2019
@tseaver tseaver changed the title Datastore retry param added in page iterator Datastore: add retry param to page iterator. Jul 8, 2019
call = retry(call)
return call()


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add explicit unit tests for _call_api, including covering the case when retry is None.

def _call_api(fnc_call, retry, *args, **kwargs):

call = functools.partial(fnc_call, *args, **kwargs)
if retry:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If retry is None, then avoide the partial and just return fnc_call(*args, **kwargs).

To modify the default retry behavior, call a ``with_XXX`` method
on ``DEFAULT_RETRY``. For example, to change the deadline to 30 seconds,
pass ``retry=bigquery.DEFAULT_RETRY.with_deadline(30)``.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just copying this file in wholesale from bigquery isn't appropriate: we should be refactoring to share this implementation.

from google.api_core.exceptions import BadGateway

exc = BadGateway("testing")
self.assertTrue(self._call_fut(exc))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here for the copy-paste from BigQuery.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 10, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 10, 2019
@tseaver
Copy link
Contributor

tseaver commented Jul 10, 2019

@pchauhan-qlogic Please run nox -e black.

This PR still needs to avoid blindly copying the BigQuery retry module: for one thing, we have no reason to believe the set of retryable reasons / exception types should be the same for the two backends.

@tswast WDYT?

@tswast
Copy link
Contributor

tswast commented Jul 10, 2019

That's right. BigQuery is definitely a special snowflake with respect to retry codes. I recommend choosing the codes to retry independently for Datastore.

@tseaver tseaver added api: datastore Issues related to the Datastore API. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 16, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019
@pchauhan-qlogic
Copy link
Contributor Author

please look into @tswast

@crwilcox
Copy link
Contributor

I think to bring this back to an earlier comment by @tseaver, we likely want shared code, but the list of retry codes is likely to be special per client.

We have a higher level work item that needs to be done which is to decide on the best way forward for providing timeout and retry functionality in our clients in a way that is consistent across products.

@tseaver
Copy link
Contributor

tseaver commented Sep 25, 2019

Overall design of retries for manual clients is tracked in #9298.

@tseaver tseaver closed this Sep 25, 2019
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. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datastore: expose retry parameters
7 participants