-
Notifications
You must be signed in to change notification settings - Fork 108
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
[close #372] add circuit breaker and smart rawkv client #358
[close #372] add circuit breaker and smart rawkv client #358
Conversation
38145a4
to
3b62a5c
Compare
/run-all-tests |
this.listeners = new ArrayList<>(); | ||
this.currentMetrics = new AtomicReference<>(new SingleWindowMetrics()); | ||
|
||
scheduler = |
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.
should this background task be managed by the circuit breaker instead of the circuit metrics?
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.
why?
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.
I think it's a subtask belonging to the circuit breaker. For the metrics, it only needs to record the traffic statistics.
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.
BTW, we should not start this background thread when the circuit breaker is configured to be disabled.
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.
fixed in 5fcf19b
src/main/java/org/tikv/service/failsafe/CircuitBreakerMetricsImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: marsishandsome <marsishandsome@gmail.com>
f42b506
to
929f3db
Compare
Signed-off-by: marsishandsome <marsishandsome@gmail.com>
2eea8ad
to
9932dab
Compare
Signed-off-by: marsishandsome <marsishandsome@gmail.com>
Signed-off-by: marsishandsome <marsishandsome@gmail.com>
this.listeners = new ArrayList<>(); | ||
this.currentMetrics = new AtomicReference<>(new SingleWindowMetrics()); | ||
|
||
scheduler = |
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.
I think it's a subtask belonging to the circuit breaker. For the metrics, it only needs to record the traffic statistics.
private final AtomicLong circuitOpened = new AtomicLong(-1); | ||
private final AtomicReference<Status> status = new AtomicReference<>(Status.CLOSED); | ||
private final AtomicLong attemptCount = new AtomicLong(0); | ||
private final AtomicLong attemptSuccessCount = new AtomicLong(0); |
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 really need to set these variables as atomic variables? I can only see they are accessed within the current TiSession.
-
we add the 'final' keyword to these fields but their values are changed during the execution time, seems a little counterintuitive.
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.
There are two-time windows and their corresponding traffic stats:
- the first time window is to record normal traffic stats when the circuit breaker is closed or disabled in the background and triggers circuit breaker, which is limited to
TiKV_CIRCUIT_BREAK_AVAILABILITY_WINDOW_IN_SECONDS
and is called windowOfClosed. - the second time window to record the attempt traffic stats when the circuit breaker is half-opened, which is called windowOfHalfOpened.
these two windows can be posited like this: windowOfClosed -> (circuit breaker opened) -> (after sleep, circuit breaker half-opened) -> windowOfHalfOpened
So can we:
- Define two traffic metrics for these two time windows like the following and remove the atomic variables for attempts:
private final CircuitBreakerMetrics metricsOfClosed;
private final CircuitBreakerMetrics metricsOfHalfOpened
- Make
CircuitBreakerImpl
be the owner of these two windows, don't start the background task to collectmetricsOfClosed
when the circuit breaker is disabled.
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 really need to set these variables as atomic variables?
yes, in order to make CircuitBreakerImpl
thread-safe.
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.
we add the 'final' keyword to these fields but their values are changed during the execution time, seems a little counterintuitive.
yes, we should add final
and the object reference does not change. The object content does change.
this.listeners = new ArrayList<>(); | ||
this.currentMetrics = new AtomicReference<>(new SingleWindowMetrics()); | ||
|
||
scheduler = |
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.
BTW, we should not start this background thread when the circuit breaker is configured to be disabled.
Signed-off-by: marsishandsome <marsishandsome@gmail.com>
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.
LGTM
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.
LGTM
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
cherry pick to release-3.1 in PR #373 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Signed-off-by: marsishandsome <marsishandsome@gmail.com>
Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Signed-off-by: marsishandsome <marsishandsome@gmail.com>
Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Signed-off-by: marsishandsome <marsishandsome@gmail.com>
Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Signed-off-by: marsishandsome <marsishandsome@gmail.com>
Signed-off-by: ti-srebot <ti-srebot@pingcap.com> Signed-off-by: marsishandsome <marsishandsome@gmail.com> Co-authored-by: Liangliang Gu <marsishandsome@gmail.com>
Signed-off-by: marsishandsome marsishandsome@gmail.com
add Circuit Breaker