Clarify retry.RetryOnConflict docs #82284
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
sig/api-machinery
|
/assign @caesarxuchao |
| // RetryOnConflict is used to make an update to a resource when you have to worry about | ||
| // conflicts caused by other code making unrelated updates to the resource at the same | ||
| // time. fn should fetch the resource to be modified, make appropriate changes to it, try | ||
| // to update it, and return (unmodified) the result from the update function. On a |
There was a problem hiding this comment.
(unmodified) the result
The "result" is the "error" returned by the update, right? If so, can we just say the "the (unmodified) error" to be more specific?
| // ... | ||
| // | ||
| // OnError is an abstraction of RetryOnConflict, which allows the caller to control which | ||
| // errors will result in a retry. | ||
| // TODO: Make Backoff an interface? | ||
| func OnError(backoff wait.Backoff, errorFunc func(error) bool, fn func() error) error { | ||
| var lastConflictErr error |
There was a problem hiding this comment.
Since you are here, could you rename this variable? It shouldn't have "conflict" in the name.
| // } | ||
| // ... | ||
| // | ||
| // OnError is an abstraction of RetryOnConflict, which allows the caller to control which |
There was a problem hiding this comment.
I think we don't need to mention "OnError is an abstraction of RetryOnConflict".
How about:
I will just say OnError allows caller to retry fn in case the error returned by fn is retriable according to errorFunc. backoff defines the maximum retries and the wait interval between two retries.
| // ... | ||
| // | ||
| // OnError is an abstraction of RetryOnConflict, which allows the caller to control which | ||
| // errors will result in a retry. | ||
| // TODO: Make Backoff an interface? | ||
| func OnError(backoff wait.Backoff, errorFunc func(error) bool, fn func() error) error { |
There was a problem hiding this comment.
Since you are here, can you rename errorFunc to retriable to be meaningful?
danwinship
force-pushed
the
retry-on-conflict-docs
branch
from
6de445b
to
23b391e
Compare
|
@caesarxuchao updated |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao, danwinship The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Please squash if you can make it. |
What this PR does / why we need it:
The documentation for
retry.RetryOnConflictis both incomplete and misleading. Especially, the example code is not even using the function correctly, and is equivalent to not usingRetryOnConflictat all (in that, if it does not succeed on the first try, it will not succeed on any of the retries either).Also, #80402 moved most of the
RetryOnConflictdocumentation to the newOnErrorfunction, but I think it makes sense to thoroughly documentRetryOnConflictitself, since it is the more commonly-used function, and because many of the details of how it needs to be used are specific to conflict errors./kind documentation
/priority important-soon