consul icon indicating copy to clipboard operation
consul copied to clipboard

feat: update rpc retry criteria

Open JadhavPoonam opened this issue 2 years ago • 2 comments

Description

This change makes the following change to the rate-limiting retry logic:

- Client RPC to Server -> Retry should be in the client, only the "ErrRetryElsewhere" error
- Client RPC to Follower Forwarded to Leader -> Retry should be implemented in the client, only the "ErrRetryElsewhere" error
- Server RPC executed locally -> Should retry, only the "ErrRetryElsewhere" error
- Server RPC forwarded to Leader -> Should retry, only "ErrRetryLater" error

PR Checklist

  • [ ] ~updated test coverage~
  • [ ] ~external facing docs updated~
  • [ ] ~not a security concern~

JadhavPoonam avatar Jan 27 '23 21:01 JadhavPoonam

Thank you @JadhavPoonam for working on this.

Looking to the change in here, I think it make the canRetry logic a bit confusing.

Another approach that I think would be better, is to extract retriableErrors as a func parameter and pass it in both cases. What you think about that?

I have passed the retryable messages separately now and is based on the following criteria:

  1. On the client side we want to retry for ErrChunkingResubmit and both rate limiting errors ErrRetryElsewhere
  2. In the forwardRequestToLeader method we retry ErrRetryLater error

JadhavPoonam avatar Feb 01 '23 18:02 JadhavPoonam

@JadhavPoonam we need to add retries in the server RPC for the rate limiter errors, this is to cover the case of direct HTTP to a server.

dhiaayachi avatar Feb 02 '23 16:02 dhiaayachi