This repository has been archived by the owner on Jun 28, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support for scheduled retry with retryAfter #94
Add support for scheduled retry with retryAfter #94
Changes from 16 commits
e676139
7e1f414
2cd04f6
9e7b028
b39cc74
916226d
55db3f0
447ba17
3a27406
7a4a032
cbc121f
45a0204
9263029
522cb32
eba4b42
96c8d3a
6afd99c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
params
object is reused across retries. Is it safe to mutate it? I prefer to clone it for each retry unless its reference equality is used somewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is a cleanup for the happy case. Is this array of callbacks cleaned up in the sad case (all requests fail)? I'm not sure if it causes a memory leak only from the code, but I think it's worth checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good catch! Will check that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to mark aborted requests as timeouts when aborting because of a successful request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging is one thing, but we also need to make sure that this doesn't open the circuit breaker when aborting because of another successful request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once one on the request has succeed, retryOperation will be marked as
resolved
which will bypass any reject "bubble", see:So actually clients are not even aware of the result of this promise. There is a vicious side effect though.: this request is actually never resolved or rejected: is it a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, so the circuit breaker command's
success()
orfailure()
are not called. Then, it won't make falsefailure()
calls at least!But isn't the circuit breaker designed with an assumption that one of
success()
orfailure()
is called when an operation ends? It seems that the circuit breaker thinks that it is a timeout ifsuccess()
orfailure()
are not called. And timeouts contribute to error rate calculation in the circuit breaker, which means it can open the circuit breaker without actual errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we should stop the timeout timer in the circuit breaker when aborting requests because of another successful request. Because calling
success()
will lower the actual error rate, how about creating another function (cancel
or something) inCommand
?We may want to mark failures in some cases though. For example, if the first request doesn't respond and the second request succeeds, it makes more sense to mark the first request as a failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow very good catch! Will evaluate adding this new function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this approach seems to work. I have to write more tests now to cover all the different cases: