-
Notifications
You must be signed in to change notification settings - Fork 4k
core: updates the backoff range as per the A6 redefinition#11858
core: updates the backoff range as per the A6 redefinition#11858larry-safran merged 5 commits intomasterfrom
Conversation
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not testing when isExperimentalRetryJitterEnabled is toggled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
shivaspeaks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for Eric's review. @ejona86