Conversation
80fa066 to
7605c1e
Compare
This commit is a continuation of a previous commit which added a utility function for use in service classes which ensures that they follow the circuit breaker and retry patterns. This commit completes the function by adding: - `retryFilterPolicy`, which allows for retrying certain errors and not handling others. - `maxRetries`, which allows for giving up early before the max consecutive failures is reached. Note that because this can combine with `maxConsecutiveFailures`, it adds a bunch of other code paths and so this requires adding a lot of new tests.
7605c1e to
66bbe94
Compare
There was a problem hiding this comment.
So... the amount of changes here are still very large, and it really screws up the diff. I probably could have pre-organized these tests up front the way they are presented now to make the diff easier to read, but regardless, there are a large number of tests being added.
I will repeat the same instructions I gave before: it's probably best to run this command to view the test names from a high level:
yarn workspace @metamask/controller-utils run jest packages/controller-utils/src/create-service-policy.test.ts --no-coverage
Or, go into VSCode, run "Fold All", and then progressively unfold the describe blocks.
There you should be able to see some patterns. First, I've divided the tests into three major scenarios:
- wrapping a service that succeeds on the first try
- wrapping a service that always fails
- wrapping a service that fails continuously and then succeeds on the final try
The 1st scenario is pretty simple; all we need to really test is that onBreak is never called, and onDegraded is sometimes called.
The 2nd scenario adds complexity, because since both maxRetries and maxConsecutiveFailures are optional, there are four code paths we have to test, and then inside that there are more code paths depending on whether the number of retries is greater than, less than, or equal to the number of consecutive failures. All of that affects whether/when onBreak or onDegraded are called.
Finally, the 3rd scenario is also important because that is where we can test that the service gets re-evaluated if it fails enough times that the circuitBreakDuration elapses. (We also test a bunch of stuff we are already testing in the 2nd scenario. Despite the duplication I thought these tests were still important so we know what happens.)
| // do nothing | ||
| }, | ||
| }: { | ||
| maxRetries?: number; |
There was a problem hiding this comment.
Should we call this retries instead of maxRetries?
I guess a better question is, how would you expect to use this function?
const policy = createServicePolicy({ maxRetries: 3 });vs:
const policy = createServicePolicy({ retries: 3 });There was a problem hiding this comment.
I would opt in to keep maxRetries for clarity and to avoid ambiguity.
There was a problem hiding this comment.
See original (outdated but still relevant) comment: #5149 (comment)
This commit is a continuation of a previous commit which added a utility function for use in service classes which ensures that they follow the circuit breaker and retry patterns. This commit completes the function by adding: - `retryFilterPolicy`, which allows for retrying certain errors and not handling others. - `maxRetries`, which allows for giving up early before the max consecutive failures is reached. Note that because this can combine with `maxConsecutiveFailures`, it adds a bunch of other code paths and so this requires adding a lot of new tests.
Explanation
This commit is a continuation of a previous commit which added a utility function for use in service classes which ensures that they follow the circuit breaker and retry patterns.
This commit completes the function by adding:
retryFilterPolicy, which allows for retrying certain errors and not handling others.maxRetries, which allows for giving up early before the max consecutive failures is reached. Note that because this can combine withmaxConsecutiveFailures, it adds a bunch of other code paths and so this requires adding a lot of new tests.References
Progresses #4994.
Changelog
(Updated in PR)
Checklist