docs: clarify behavior of hedge_on_per_try_timeout

[ikonst]

No description provided.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12983 was opened by ikonst.

see: more, trace.

[ikonst]

@snowp I had a hard time understanding the original phrasing so I took a stab at rewriting it, but I'm not really sure I'm describing how it actually works.

[snowp]

Thanks for improving the docs!

@mpuncel You wanna give this a look before I do?

@ikonst Can you fix DCO? There are steps to do so in CONTRIBUTING.md

[stale]

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!

[ikonst]

bump @mpuncel

[snowp]

Thanks for improving the docs!

I think you'll also want to update the V3 docs, V2 is on its way out. The V4alpha docs should then be automatically updated once you run the proto_format.sh script.

[mpuncel]

I don't think this part is right, retry policy doesn't matter when per try timeout is hit, only whether hedge_on_per_try_timeout is set

[mpuncel]

reference comment:

RetryStatus RetryStateImpl::shouldHedgeRetryPerTryTimeout(DoRetryCallback callback) {
  // A hedged retry on per try timeout is always retried if there are retries
  // left. NOTE: this is a bit different than non-hedged per try timeouts which
  // are only retried if the applicable retry policy specifies either
  // RETRY_ON_5XX or RETRY_ON_GATEWAY_ERROR. This is because these types of
  // retries are associated with a stream reset which is analogous to a gateway
  // error. When hedging on per try timeout is enabled, however, there is no
  // stream reset.
  return shouldRetry(true, callback);
}

(the comment wording there is also confusing!)

[ikonst]

Interesting. I was actually working off my experiencing using this feature: I only started seeing (hedged) retries once I set x-envoy-retry-on.

[ikonst]

Looking at this code, it seems that retries_remaining_ would only get initialized to a non-zero value if retry_on_ is set:

envoy/source/common/router/retry_state_impl.cc

Lines 121 to 127 in 2709b6b

	if (retry_on_ != 0 && request_headers.EnvoyMaxRetries()) {
	uint64_t temp;
	if (absl::SimpleAtoi(request_headers.getEnvoyMaxRetriesValue(), &temp)) {
	// The max retries header takes precedence if set.
	retries_remaining_ = temp;
	}
	}

Thus shouldRetry would end up returning RetryStatus::NoRetryLimitExceeded for the hedged requests.

I can imagine this wasn't intended, since it surprised me too, but on the other hand it's congruent with how non-hedged retrying behaves.

[mpuncel]

I think this might be clearer stated as

"Any response received after the timeout and subsequent hedge attempt will never be retried, no matter the RetryPolicy"

[ikonst]

Assuming timeout 150ms, per-try timeout of 50ms, 3 retries and retry-on: 5xx policy, and hedging enabled:

0ms: Request 1 sent.
50ms: Request 1 times out, (hedged) request 2 sent.
75ms: Request 2 (hedged) returns 500.
150ms: Request 1 times out.

Would there be a 3rd request?

(for simplicity assuming no exponential backoff in those timings)

[mpuncel]

there would be a 3rd request. Request 2 is considered a new attempt, so it will be retried if it times out or returns a 500. If request 1 comes back with a 500 after request 2 has already been sent, that will be dropped and not retried

[ikonst]

ahh, I read (Any response received after the timeout) AND (subsequent hedge attempt) -> will never be retried 🤦

[mpuncel]

i don't think this is true, you don't need to have gateway-error to have it be retried. A per try timeout is always retried when hedging is enabled

[ikonst]

@mpuncel

I tested it and found it to be true. (In fact, wasted quite some time trying to understand why hedging didn't work for me before I tried adding a simple x-envoy-retry-on to my calls.).

I actually wrote this above in reply to a similar question you raised:

Looking at this code, it seems that retries_remaining_ would only get initialized to a non-zero value if retry_on_ is set:

envoy/source/common/router/retry_state_impl.cc

Lines 121 to 127 in 2709b6b

	if (retry_on_ != 0 && request_headers.EnvoyMaxRetries()) {
	uint64_t temp;
	if (absl::SimpleAtoi(request_headers.getEnvoyMaxRetriesValue(), &temp)) {
	// The max retries header takes precedence if set.
	retries_remaining_ = temp;
	}
	}

Thus shouldRetry would end up returning RetryStatus::NoRetryLimitExceeded for the hedged requests.

I can imagine this wasn't intended, since it surprised me too, but on the other hand it's congruent with how non-hedged retrying behaves.

[mpuncel]

interesting! I do think that isn't intentional. In that case I'm not really sure how to word the comment, maybe you could say "you must have a RetryPolicy that retries at least one error code and specify the max number of retries". You don't have to have gateway-error specifically though it looks like from that code snippet

[snowp]

@ikonst I think this change looks good now. Could you a) merge master b) fix the formatting issue and c) apply the same change to the V3 docs (then run tools/proto_format.sh fix to propagate to v4alpha)?

Also friendly reminder that force pushing breaks the reviewing flow for many, so avoid it if you can.

Thanks!

[ikonst]

I'm not a fan of rebases either since they break the already-reviewed / new-commits separation. (Perhaps I did this one because I forgot some sign-offs mid-way?)

[ikonst]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #12983 (comment) was created by @ikonst.

see: more, trace.

[ikonst]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #12983 (comment) was created by @ikonst.

see: more, trace.

[ikonst]

@snowp ^

[snowp]

LGTM, thanks!

@envoyproxy/api-shepherds for API review and V2 sign off

[htuch]

/lgtm v2-freeze
/lgtm api