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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the flow of request to fix #1638 for circuit breakers #1645
Refactor the flow of request to fix #1638 for circuit breakers #1645
Changes from 2 commits
7c99c46
b2a78be
ddd62dd
a39d440
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.
Actually IMHO "complete" might be more concrete. In Sentinel, "pass" means allowed by Sentinel (not started but pending), and "complete" means an invocation has completed (the blocked requests never started, so never completed).
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.
And the
ResourceWrapper
seems unused? (it could be taken fromEntry
inContext
)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.
Yeah naming issues often have more than one answer mostly. I tried to make it clear for requests passed by sentinel(
passed
) and it will be invoked as it completed(after
). Surely we can have other picks. IsonRequestComplete
implicit whether sentinel guarantees it would be called no matter whether it was blocked?Talk to the parameters in definition it just follows similar designs in related logics and try to make it more stable in future iterations.
ResourceWrapper
will contains the resource information which it's useful especially in implementations in future. My opinion is more like to keep it as stability of interface.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.
Originally the semantics of
onComplete
means the invocation has completed, so it MUST have passed Sentinel's checking. The rejected invocations never passed, so it would never complete.Okay for me. Actually I'd like the circuit breaker to be a more generic design (e.g. the context could be generic, and metadata could be retrieved from it).
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.
Naming of callback has been reverted and it has more comments on it.
Talk to the generic context the idea makes sense but it also may introduce more memory footprints. Maybe it can be decided in future.
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 we may discuss it later in a new issue.