core: updates the backoff range as per the A6 redefinition #11858
Conversation
…as per the A6 redefinition
AgraVator
requested review from
ejona86,
kannanjgithub,
shivaspeaks and
larry-safran
| // Calculate the exponential backoff delay with jitter | ||
| double exponent = retryCount > 0 ? Math.pow(BACKOFF_MULTIPLIER, retryCount) : 1; | ||
| long delay = (long) (INITIAL_BACKOFF_IN_SECONDS * exponent); | ||
| return RetriableStream.intervalWithJitter(delay); |
There was a problem hiding this comment.
This is not testing when isExperimentalRetryJitterEnabled is toggled.
There was a problem hiding this comment.
Yes, had discussed it with @kannanjgithub, we can add it if it is absolutely needed, but the default behavior is covered and the other one is to be deprecated after a few releases.
P.S. I think you meant the testing is not covered for "false" or the old behavior.
There was a problem hiding this comment.
In this case it is pretty trivial to see that setting the flag to false couldn't cause any problems, so it is fine to skip testing that case.
If it wasn't so obvious, but still simple, you'd want to create a single test case where you:
- saved the flag's System.property value (could be null)
- set it to the non-default value (in this case "false")
- Do the test logic
- restore the System.property (for null do an unset)
Then when the flag is removed the test case can be deleted (would want to add a TODO on the test case indicating that it should be removed that includes the flag name).
If it was something complicated that affected a lot of behavior then you would want to parameterize the test so that all (or at least most) of the test cases would run both ways. You can do a search for @RunWith(Parameterized.class) to see a number of examples in our existing tests. Note that to skip specific tests with some options you can use org.junit.Assume
| // Calculate the exponential backoff delay with jitter | ||
| double exponent = retryCount > 0 ? Math.pow(BACKOFF_MULTIPLIER, retryCount) : 1; | ||
| long delay = (long) (INITIAL_BACKOFF_IN_SECONDS * exponent); | ||
| return RetriableStream.intervalWithJitter(delay); |
There was a problem hiding this comment.
In this case it is pretty trivial to see that setting the flag to false couldn't cause any problems, so it is fine to skip testing that case.
If it wasn't so obvious, but still simple, you'd want to create a single test case where you:
- saved the flag's System.property value (could be null)
- set it to the non-default value (in this case "false")
- Do the test logic
- restore the System.property (for null do an unset)
Then when the flag is removed the test case can be deleted (would want to add a TODO on the test case indicating that it should be removed that includes the flag name).
If it was something complicated that affected a lot of behavior then you would want to parameterize the test so that all (or at least most) of the test cases would run both ways. You can do a search for @RunWith(Parameterized.class) to see a number of examples in our existing tests. Note that to skip specific tests with some options you can use org.junit.Assume
This is the only usage of PickSubchannelArgs when creating a filter's ClientInterceptor, and a follow-up commit will remove the argument and actually reuse the interceptors. Other filter's interceptors can already be reused. There doesn't seem to be any significant loss of legibility by making FaultFilter a more ordinary interceptor, but the change does cause the ForwardingClientCall to be present when faultDelay is configured, independent of whether the fault delay ends up being triggered. Reusing interceptors will move more state management out of the RPC path which will be more relevant with RLQS.
* netty: Removed 4096 min buffer size
Fixes: #11700
Updates the backoff range from [0, 1] to [0.8, 1.2] as per the A6 redefinition