Repository navigation
fix(delivery): wait one backoff delay before the first linear retry - #9425
Conversation
knative-prow
Bot
added
the
size/M
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9425 +/- ##
=======================================
Coverage 51.33% 51.33%
=======================================
Files 411 411
Lines 22187 22187
=======================================
Hits 11389 11389
Misses 9916 9916
Partials 882 882 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ring The JetStream channel CRD describes the exponential policy as backoffDelay*2^<numberOfRetries>, but since the retry attempt numbering was aligned with Knative's zero-based callback, the first retry waits backoffDelay (2^0) rather than 2*backoffDelay. Describe it as backoffDelay*2^(<numberOfRetries>-1) with <numberOfRetries> being one-based. This mirrors the wording change in knative/eventing#9425. The linear sentence is unchanged: it becomes accurate for the first retry once that PR is picked up. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: kahirokunn <okinakahiro@gmail.com>
The CRD documents the linear policy as backoffDelay*<numberOfRetries>, so the first retry should wait one backoffDelay and the second two. The vendored knative.dev/eventing still returns 0 for the first linear retry (attemptNum 0), so these two rows fail until knative/eventing#9425 is picked up. They are added ahead of that dependency update on purpose so the intended behavior is pinned down; verified that they pass when the #9425 linearBackoff change is applied to the vendored copy. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: kahirokunn <okinakahiro@gmail.com>
801e64c to
46778b2
Compare
Signed-off-by: kahirokunn <okinakahiro@gmail.com>
46778b2 to
821361d
Compare
|
/retest |
|
CI is failing due to an issue on the Kafka side. 😢 |
| properties: | ||
| backoffDelay: | ||
| description: 'BackoffDelay is the delay before retrying. More information on Duration format: - https://www.iso.org/iso-8601-date-and-time-format.html - https://en.wikipedia.org/wiki/ISO_8601 For linear policy, backoff delay is backoffDelay*<numberOfRetries>. For exponential policy, backoff delay is backoffDelay*2^<numberOfRetries>.' | ||
| description: 'BackoffDelay is the delay before retrying. More information on Duration format: - https://www.iso.org/iso-8601-date-and-time-format.html - https://en.wikipedia.org/wiki/ISO_8601 For linear policy, backoff delay is backoffDelay*<numberOfRetries>. For exponential policy, backoff delay is backoffDelay*2^(<numberOfRetries>-1).' |
There was a problem hiding this comment.
@kahirokunn iiuc the scope of this PR is mostly fixing the linear retry logic. What is the reason for changing the exponential backoff docs in this PR as well?
There was a problem hiding this comment.
@Cali0707
Thanks for your review 🙏
I included this to use one-based retry numbering consistently in both formulas. Exponential already starts at backoffDelay , so this only corrects the docs without changing behavior.
There was a problem hiding this comment.
@Cali0707 we came to the conclusion docs we not accurate based on a parallel review for backoff delay logic.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, kahirokunn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
approved
Fixes #
With
backoffPolicy: linearthe first retry is sent immediately, without waiting at all.go-retryablehttp calls
Backoffwith a zero-based attempt number (0 is the wait before the first retry), butlinearBackoffreturneddelay * attemptNum, so attempt 0 gave 0. Exponential isn't affected, it already waitsbackoffDelayfirst. WithbackoffDelay: PT1Sthe waits between attempts were 0s, 1s, 2s for linear and are 1s, 2s, 3s now.The intended behavior from #4232 / #4240 is
backoffDelay * <numberOfRetries>. The existing tests didn't catch it because they calledBackoffstarting from 1, which isn't what retryablehttp does.Proposed Changes
linearBackoffreturndelay * (attemptNum + 1)so the first retry waitsbackoffDelayThis means every retry with the linear policy waits one
backoffDelaylonger than before. Anything callingRetryConfig.Backoffdirectly with a one-based number gets a result one step too large for linear.Pre-review Checklist
Release Note
Docs