HTTP Retry Support - #255
Conversation
schmidt-sebastian
left a comment
There was a problem hiding this comment.
This is some high quality code! :) The comments I left are mainly there to push you to re-use more GAX classes. Can you see if we can use their settings objects? That might be more elegant that spinning our own objects as intermediaries.
I haven't yet looked at the tests.
| import static com.google.common.base.Preconditions.checkArgument; | ||
| import static com.google.common.base.Preconditions.checkNotNull; | ||
|
|
||
| import com.google.api.client.util.BackOff; |
There was a problem hiding this comment.
| /** | ||
| * Sets a list of HTTP status codes that should be retried. If null or empty, HTTP requests | ||
| * will not be retried as long as they result in some HTTP response message. I/O errors | ||
| * will still be retried. |
There was a problem hiding this comment.
We have had some long discussion with the Firestore backend team about retrying requests that were dropped and never responded to. Unless we know for certain that no byte made it across the wire, it was decided that we cannot retry. Since you are essentially building a retry framework for a wide variety of consumers, I would advise that we don't retry I/O errors by default.
There was a problem hiding this comment.
In Python and Go we have enabled this, but I can see why this can be a bad idea in some cases. Added an option to make this configurable via RetryConfig.
There was a problem hiding this comment.
Please update comment to state that this is only true if setRetryOnIOExceptions is enabled.
|
Thanks @schmidt-sebastian. I've made some changes and responded to your comments. In general, I don't see how to reuse gax retry mechanism in our code, since we don't use gax. But I've managed to make most of the other improvements suggested. |
schmidt-sebastian
left a comment
There was a problem hiding this comment.
Thanks for getting back to me on my comments. While I do think we could go out of our way here and use existing retry logic, I can certainly understand your reasoning. Otherwise, this PR looks great. I have also since looked at the tests and am signing off on those as well. I left some nits, but nothing major.
| /** | ||
| * Sets a list of HTTP status codes that should be retried. If null or empty, HTTP requests | ||
| * will not be retried as long as they result in some HTTP response message. I/O errors | ||
| * will still be retried. |
There was a problem hiding this comment.
Please update comment to state that this is only true if setRetryOnIOExceptions is enabled.
|
|
||
| assertEquals(10, sleeper.getCount()); | ||
| assertArrayEquals( | ||
| new long[]{500, 1000, 2000, 4000, 8000, 16000, 32000, 64000, 120000, 120000}, |
There was a problem hiding this comment.
There are quite a few tests in this PR that test the same feature at different levels (see testExponentialBackOff). You can certainly leave this as is, but it might be worth considering to reduce duplication in future PRs.
* Copied DateUtils source from Apache HC * Updated reference link * Used locks instead of thread locals; Added tests * Added a NOTICE file for third-party code
…ava into hkj-http-retry
We are in the process of implementing HTTP retry support across all Admin SDKs. Specifically, we want to retry an HTTP request in the following situations:
This PR lays the foundation for supporting HTTP retries in the Java Admin SDK. However, we are not enabling retries for any of the APIs just yet. This will need some more refactoring in each of the modules, and therefore we will do it later in an incremental fashion.