client: use interceptor for circuit breaker #8936
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
release-note-none
rleungx
changed the title
ti-chi-bot
bot
added
the
size/L
| case opt.RegionMetadataCircuitBreakerSettings: | ||
| applySettingsChange, ok := value.(func(config *cb.Settings)) | ||
| if !ok { | ||
| return errors.New("[pd] invalid value type for RegionMetadataCircuitBreakerSettings option, it should be pd.Settings") | ||
| } | ||
| c.inner.regionMetaCircuitBreaker.ChangeSettings(applySettingsChange) |
There was a problem hiding this comment.
@rleungx how do you plan to update circuit breaker settings? Would it be defined at the caller layer (tidb) and passed through the context, so we won't need to plumb ChangeSettings at the client level at all?
There was a problem hiding this comment.
Yes, the circuit breaker can be defined in the caller layer. Once TiDB variable is changed, it can update the circuit breaker in the caller layer and we don't need to change the client. What do you think?
There was a problem hiding this comment.
sure, I just confirming that I've read the PR intent correctly.
| type cbCtxKey struct{} | ||
|
|
||
| // Key used to store circuit breaker | ||
| var CircuitBreakerKey = cbCtxKey{} |
There was a problem hiding this comment.
Do you foresee the need have multiple circuit breakers per context for different operations?
While this provide a lot flexibility, asking caller to provide the target circuit breaker for each operation is a bit cumbersome and easy to miss on each call path.
There was a problem hiding this comment.
It's just another way to do it. If it's complicated, I'm ok to keep the status quo.
There was a problem hiding this comment.
pros:
- align with the retry mechanism
- use the interceptor, the order is easier to control
- it can be changed to request level instead of client level
- we can only modify TiDB to add a new type if we remove the generic
cons:
- it's more complicated, each caller needs to maintain the circuit breaker
- client level is easier to maintain
There was a problem hiding this comment.
I really like the approach with interceptor and context as it is a way more elegant. The only concern that it would be easy to miss to pass a circuit break into a context when a new invocation is added at the client layer. Do your think that over time we can enforce presence of the circuit breaker in context for certain calls like get region?
There was a problem hiding this comment.
It may not be very convenient for the caller to switch during configuration changes.🤔
There was a problem hiding this comment.
Maybe we can get it directly from the SystemVar?
There was a problem hiding this comment.
We need switch it for every client.
There was a problem hiding this comment.
What if we use a global variable?
Signed-off-by: Ryan Leung <[email protected]>
rleungx
force-pushed
the
use-interceptor
branch
from
f1aeab1 to
579f67b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8936 +/- ##
==========================================
+ Coverage 76.22% 76.27% +0.05%
==========================================
Files 461 463 +2
Lines 70392 70476 +84
==========================================
+ Hits 53658 53758 +100
+ Misses 13389 13362 -27
- Partials 3345 3356 +11
Flags with carried forward coverage won't be shown. Click here to find out more. |
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
ti-chi-bot
bot
added
size/XL
rleungx
force-pushed
the
use-interceptor
branch
2 times, most recently
from
9b34b99 to
420781e
Compare
Signed-off-by: Ryan Leung <[email protected]>
rleungx
force-pushed
the
use-interceptor
branch
from
420781e to
13d663d
Compare
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: okJiang The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: ref #8678
What is changed and how does it work?
This PR uses an interceptor to provide circuit breaker functionality. The pros and cons of this way can be found at #8936 (comment)
Check List
Tests
Release note