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

Enable retries in botocore. #970

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tasn
Copy link

@tasn tasn commented Aug 11, 2021

Botocore has built-in retries, we should probably use them rather than reimplement in pynamodb, no?

Any thoughts?

@ikonst
Copy link
Contributor

ikonst commented Aug 12, 2021

Unless we have firm intentions of going off botocore (see #968), I'd see this as a positive change. As you can see in the docs for Standard retry mode, botocore has logic around which exceptions are transient and good candidates for retries, as well as exponential backoff.

WDYT @garrettheel?

@tasn
Copy link
Author

tasn commented Aug 12, 2021

I'm happy to improve the patch a bit to get it "ready" (cleaning up the for loop), though I'd need help to make sure it actually retries, as I'm not sure what's the right way to be doing such testing.

Also, I think that this is a plus even if we end up turning away from botcore, because this is probably a good place to do it anyway, so even in whatever abstraction we come up with, it should maybe be there.

@ikonst
Copy link
Contributor

ikonst commented Aug 12, 2021

though I'd need help to make sure it actually retries, as I'm not sure what's the right way to be doing such testing

A unit test wouldn't need to test it, since we'd rely on botocore's own unit tests to make sure its retries work well.

For an integration test... maybe write a simple server with http.server that'd proxy to the underlying dynamodb-local, and make it fail the 1st request in a predictable way?

@tasn
Copy link
Author

tasn commented Aug 12, 2021

I'm even unsure about manual testing as I don't know how a retry-worthy response looks like from dynamo.

@garrettheel
Copy link
Member

Unless we have firm intentions of going off botocore (see #968), I'd see this as a positive change. As you can see in the docs for Standard retry mode, botocore has logic around which exceptions are transient and good candidates for retries, as well as exponential backoff.

WDYT @garrettheel?

Yep, this sounds good to me. Let's just try to avoid exposing the botocore interface too much so as not to shut the door to replacing the client implementation.

@tasn
Copy link
Author

tasn commented Oct 27, 2021

One problem that we've noticed is that (and I'm not sure about this) - this change maybe doesn't retry in all of the same scenarios. Is there a way to sanely add tests for these things to make sure we are good?

@garrettheel
Copy link
Member

One problem that we've noticed is that (and I'm not sure about this) - this change maybe doesn't retry in all of the same scenarios. Is there a way to sanely add tests for these things to make sure we are good?

I don't know that we need tests exactly, but I think it would be helpful to do some analysis of the botocore standard behavior vs. what is already happening here and document it in the PR so we can determine whether it would constitute a breaking change to anyone and handle appropriately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants