CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

I think I was imagining the scope of this extension to be a bit bigger, eventually making it able to express the entire current RetryPolicy, including managing retry attempts if desirable (e.g. someone might want certain failures to not count against the retry count).

How about something that looks like this:

class RetryPolicyFactory {
  RetryPolicySharedPtr createRetryPolicy(const Http::HeaderMap& response_headers, <config>);
};

class RetryPolicy {
  // Called when an upstream request has been completed with headers.
  void recordResponseHeaders(const Http::HeaderMap& response_headers);
  // Called when an upstream request failed due to a reset.
  void recordReset(ResetReason reset);

  bool shouldRetry() const; 
  
  // Might need something like this to support the existing hedging behavior, can probably 
  // skip for now and not call this during the hedging path.
  bool wouldRetry(const Http::HeaderMap& response_headers) const;
};

With this kind of interface its now capable of tracking the lifetime of multiple requests, so retry policies that might want to do dynamic retry count based on what happened during the attempts could be implemented.

The key thing to notice is that the router might call it multiple times to determine whether something should or would have been be retried (in order to accept one of multiple hedged requests), so splitting up recording a response with the decision makes it possible to reason about the individual attempts.

Then we can probably update the shouldRetryX in RetryStateImpl to something like

RetryStatus RetryStateImpl::shouldRetryHeaders(const Http::HeaderMap& response_headers,
                                               DoRetryCallback callback) {
  retry_policy_->recordResponseHeaders(response_headers);
  return shouldRetry(retry_policy_.shouldRetry() || wouldRetryFromHeaders(response_headers), callback);
}

My envisioned end goal is basically to be able to express the entire current RetryPolicy as a combination of such extensions, reducing the complexity of the default retry handling while making it more expressive for power users. Hopefully this makes sense!

@mattklein123 @alyssawilk might have thoughts as well

I think I was imagining the scope of this extension to be a bit bigger, eventually making it able to express the entire current RetryPolicy, including managing retry attempts if desirable (e.g. someone might want certain failures to not count against the retry count).

That totally makes sense. I misunderstood and thought that we'd like to have another retry plugin. I agree with the envision to make the whole retry policy pluggable.

How about something that looks like this:

class RetryPolicyFactory {
  RetryPolicySharedPtr createRetryPolicy(const Http::HeaderMap& response_headers, <config>);
};

class RetryPolicy {
  // Called when an upstream request has been completed with headers.
  void recordResponseHeaders(const Http::HeaderMap& response_headers);
  // Called when an upstream request failed due to a reset.
  void recordReset(ResetReason reset);

  bool shouldRetry() const; 
  
  // Might need something like this to support the existing hedging behavior, can probably 
  // skip for now and not call this during the hedging path.
  bool wouldRetry(const Http::HeaderMap& response_headers) const;
};

With this kind of interface its now capable of tracking the lifetime of multiple requests, so retry policies that might want to do dynamic retry count based on what happened during the attempts could be implemented.

I didn't get the meaning of tracking the lifetime of multiple requests. My understanding is the pluggable retry policy will be initialized in the RetryStateImpl which is bound to one request. Do you mean the multiple request the first request + multiple retried requests?

Should the create factory interface be:

class RetryPolicyFactory {
  RetryPolicySharedPtr createRetryPolicy(const Http::HeaderMap& request_headers, <config>);
};

since the retry policy will consume response_header in the member function?

By tracking requests I mean upstream requests, i.e. each retry attempt. Right now the retry state logic is immutable, so calling wouldRetryHeaders multiple times return a consistent answer. If we want to let the retry policy modify its behavior as it sees more attempts come back, we'll want some kind of explicit API contract for that.

re the request headers yeah my thinking was that all the state that is known before the upstream requests are made should be passed to the factory, as it makes it possible to do some neat optimizations (eg return a noop RetryPolicy when the request headers imply that the request should not be retried)

I made some change based on the discussion. I didn't added

  // Might need something like this to support the existing hedging behavior, can probably 
  // skip for now and not call this during the hedging path.
  bool wouldRetry(const Http::HeaderMap& response_headers) const;

since I haven't fully understand what it's used for. If it looks good to you, I am going to work on test and doc. @snowp

I remove the retry policy extension implementation from the PR and keep the API only for review. I can send another PR to implement the API after this one gets merged.

I agree on the point that allowing extension retry policy and core retry policy cause a confusing API for users. Let me refactor the PR

1. Make the core retry policy and extension exclusive in the API

message RetryPolicy{
// RetryPolicy definition
}
message RetryPolicyExtension{
// RetryPolicyExtension definition
}
message RouteAction{
  ...
  oneof retry_policy_speficier{
    RetryPolicy retry_policy = x;
    RetryPolicyExtension retry_policy_extension = y;
  }
}

2. Move functions to retry policy extension to enable replacement for the core retry policy.

class RetryPolicyExtension{
  /*
   * Record headers and reset during the request lifecycle.
   */
  virtual void recordHeader(const Http::HeaderMap& response_headers);
  virtual void recordReset(const Http::StreamResetReason reset_reason);
  
  /*
   * Functions for querying the retry decision.
   */
  virtual bool wouldRetryFromHeaders() PURE;
  virtual bool wouldRetryFromReset() PURE;

  /* 
   * Functions for retry plugins.
   */
  virtual void onHostAttempted(Upstream::HostDescriptionConstSharedPtr host) PURE;
  virtual bool shouldSelectAnotherHost(const Upstream::Host& host) PURE;
  virtual const Upstream::HealthyAndDegradedLoad& priorityLoadForRetry
        (const Upstream::PrioritySet& priority_set, const Upstream::HealthyAndDegradedLoad& original_priority_load) PURE;
  virtual uint32_t hostSelectionMaxAttempts() const PURE;

  /*
   * Retry remainings.
   */
  virtual uint32_t retriesRemaining() PURE;

  /*
   * Retry backoff.
   */
   virtual std::unique_ptr<BackOffStrategy> backoffStrategy() PURE;

  /*
   * Per try timeout.
   */
  virtual std::chrono::milliseconds perTryTimeout() const PURE;
};

WDYT @mattklein123 @snowp

Yes ^ looks great and is I think what I would expect from an API/user perspective.

I just push the commit to refactor the RetryPolicy code and I haven't changed the test. The change seems to be huge. I am not sure if it's on the right track. I think splitting the PR into several small PRs would be helpful for review. Here is what I thought:

1. Add RetryPolicy interface and change current core retry policy to implement the RetryPolicy. The core retry policy (currently RetryPolicy, maybe renamed to CoreRetryPolicy) and retry policy extension will both implement the same interface.
2. Add RetryPolicyExtension API (this could be merged with the first step)
3. Integrate retry policy extension with the retry_state, router and config_impl, etc.
4. Add retry policy extension example

WDYT? @mattklein123 @snowp

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!