core: add perAttemptRecvTimeout to retry policy - #8301
Conversation
ef5d6f2 to
284a287
Compare
284a287 to
3d54e9f
Compare
| Set<Code> retryableCodes = | ||
| ServiceConfigUtil.getRetryableStatusCodesFromRetryPolicy(retryPolicy); | ||
| checkArgument( | ||
| perAttemptRecvTimeout != null || !retryableCodes.isEmpty(), |
There was a problem hiding this comment.
No new tests? Seems we need tests for these cases.
There was a problem hiding this comment.
Why do we need tests for assertions. They will throw if the app is in wrong state and people will notice.
There was a problem hiding this comment.
Say if I had a bug here and the code is like
checkArgument(
perAttemptRecvTimeout != null && !retryableCodes.isEmpty()
with && not ||. Then existing code going through this code path would throw.
There was a problem hiding this comment.
Then existing code going through this code path would throw.
The unit tests don't look like they would fail. If we can pass one or the other, it seems there should be tests for passing one or the other. I wouldn't be such a stickler but this is for service config and a bug in the logic can poison the well.
| } | ||
|
|
||
| // For now we allow perAttemptRecvTimeout being zero although it does not make sense. | ||
| // TODO(zdapeng): disallow zero perAttemptRecvTimeout if hedging is not enabled once we support |
There was a problem hiding this comment.
Once retries are no longer experimental, we can't be adding a restriction. Right? And it sounded like your plan was to try to get retries stable here before supporting hedging.
There was a problem hiding this comment.
Once retries are no longer experimental, we can't be adding a restriction. Right?
That's right.
And it sounded like your plan was to try to get retries stable here before supporting hedging.
Why? I'm not.
I'm currently allowing zero perAttemptRecvTimeout, because in the future, we have usecase for hedging.
Once hedge_on_per_try_timeout is supported, we will add restriction for zero perAttemptRecvTimeout without hedge_on_per_try_timeout=true. The planned change is still before getting retries stable.
2 participants
No description provided.