refactor(util): make retry policy explicit and deterministic#633
refactor(util): make retry policy explicit and deterministic#633wgtmac wants to merge 2 commits intoapache:mainfrom
Conversation
| .max_wait_ms = 10, | ||
| .total_timeout_ms = 5000}) | ||
| .OnlyRetryOn(ErrorKind::kCommitFailed) | ||
| .StopRetryOn({ErrorKind::kCommitFailed}) |
There was a problem hiding this comment.
I see StopRetryOn doesn't have a single error type signature like OnlyRetryOn, is that by design?
kamcheungting-db
left a comment
kamcheungting-db
left a comment
There was a problem hiding this comment.
Nice factor.
I have some non-blocker comments for understanding the underlying intention.
| enum class RetryPolicyMode { | ||
| kUnset, | ||
| kOnlyRetryOn, | ||
| kStopRetryOn, | ||
| }; |
There was a problem hiding this comment.
could you add one line comments for these RetryPolicyModes' behavior?
|
|
||
| if (!WaitForNextAttempt(attempt, deadline)) { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
should we record the result detail for each failed run?
| bool HasTimedOut(const std::optional<TimePoint>& deadline) const { | ||
| return deadline.has_value() && Clock::now() >= *deadline; | ||
| } | ||
| Status ValidateConfig() const; |
There was a problem hiding this comment.
please add simple comments for this method. Its implementation is not trivial
| config_.num_retries); | ||
| } | ||
| if (config_.num_retries == 0) { | ||
| return {}; |
There was a problem hiding this comment.
nitpick: could we explicitly call out?
| return {}; | |
| return RetryExhausted(<".... list of failed run reason">); |
| struct RetryTestHooks { | ||
| using Clock = std::chrono::steady_clock; | ||
| using Duration = std::chrono::milliseconds; | ||
| using TimePoint = Clock::time_point; |
There was a problem hiding this comment.
these are duplicated with these few line code
Have we considered about deduplicating them?
3 participants
No description provided.