client: introduce circuit breaker for region calls #8856

Tema · 2024-11-26T05:30:10Z

What problem does this PR solve?

What is changed and how does it work?

This PR implements circuit breaker from tikv/rfcs#115 and integrates it into http and grpc PD clients for get region metadata calls, but disabled by default.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: artem_danilov <[email protected]>

ti-chi-bot · 2024-11-26T05:30:20Z

Hi @Tema. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Signed-off-by: artem_danilov <[email protected]>

rleungx · 2024-11-26T06:20:43Z

/ok-to-test

Signed-off-by: artem_danilov <[email protected]>

rleungx · 2024-11-28T03:03:10Z

client/circuit_breaker/circuit_breaker.go

+var ErrOpenState = errors.New("circuit breaker is open")
+
+// Overloading is a type describing service return value
+type Overloading int


How about just using bool?

rleungx · 2024-11-28T03:13:28Z

client/http/interface.go

@@ -541,6 +543,11 @@ func (c *client) GetReplicateConfig(ctx context.Context) (map[string]any, error)
 	return config, nil
 }

+// UpdateCircuitBreakerSettings updates config for circuit breaker


Let's focus on gRPC calls in this PR.

sure, I can stash http client changes for now

rleungx · 2024-11-28T03:23:06Z

client/circuit_breaker/circuit_breaker.go

+)
+
+// ErrOpenState is returned when the CircuitBreaker is open or half-open with pending requests.
+var ErrOpenState = errors.New("circuit breaker is open")


How about moving it to errs/errno.go?

rleungx · 2024-11-28T03:25:30Z

client/circuit_breaker/circuit_breaker.go

+	state *State[T]
+
+	successCounter  prometheus.Counter
+	failureCounter  prometheus.Counter


When will it be used?

failureCounter should be used next to successCounter in onResult instead of fastFailCounter.
The idea of having these metrics is to see overall error rate over time to decide on ErrorRateThresholdPct value to set/overrie

I don't find where it is used in the current codebase.

Tema · 2024-11-28T05:01:31Z

@rleungx can you help to take a look at https://github.com/tikv/pd/actions/runs/12043529055/job/33580779593?pr=8856 as I don't see why it marks it as FAILED while as there is no FAIL in the tests.

rleungx · 2024-11-28T08:09:09Z

@rleungx can you help to take a look at https://github.com/tikv/pd/actions/runs/12043529055/job/33580779593?pr=8856 as I don't see why it marks it as FAILED while as there is no FAIL in the tests.

=== RUN   TestCircuitBreaker_OpenState_Not_Enough_QPS
    circuit_breaker_test.go:58: 
        	Error Trace:	/home/runner/work/pd/pd/client/circuit_breaker/circuit_breaker_test.go:58
        	Error:      	Not equal: 
        	            	expected: circuit_breaker.StateType(0)
        	            	actual  : *circuit_breaker.State[int](&circuit_breaker.State[int]{stateType:0, cb:(*circuit_breaker.CircuitBreaker[int])(0xc00012e7e0), end:time.Time{wall:0xc1c9c8f10d7e21ae, ext:30011969707, loc:(*time.Location)(0xd37300)}, pendingCount:0x0, successCount:0x0, failureCount:0x0})
        	Test:       	TestCircuitBreaker_OpenState_Not_Enough_QPS
--- FAIL: TestCircuitBreaker_OpenState_Not_Enough_QPS (0.00s)

Signed-off-by: artem_danilov <[email protected]>

codecov · 2024-11-29T01:51:07Z

Codecov Report

Attention: Patch coverage is 80.80808% with 38 lines in your changes missing coverage. Please review.

Project coverage is 76.19%. Comparing base (7df0ff9) to head (4c0cc9e).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8856      +/-   ##
==========================================
- Coverage   76.21%   76.19%   -0.03%     
==========================================
  Files         460      461       +1     
  Lines       70239    70431     +192     
==========================================
+ Hits        53532    53662     +130     
- Misses      13362    13409      +47     
- Partials     3345     3360      +15

Flag	Coverage Δ
unittests	`76.19% <80.80%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Tema · 2024-11-29T02:09:54Z

@rleungx thanks for the comments. I think I've addressed all of those and also fixed all test and lint failures.

client/circuitbreaker/circuit_breaker.go

client/client.go

client/circuitbreaker/circuit_breaker.go

okJiang · 2024-11-29T08:28:57Z

client/circuitbreaker/circuit_breaker_test.go

+	cb := NewCircuitBreaker[int]("test_cb", settings)
+	re.Equal(StateClosed, cb.state.stateType)
+	driveQPS(cb, minCountToOpen, Yes, re)
+	cb.advance(settings.ErrorRateWindow)
+	assertFastFail(cb, re)


How about wraping a function to reduce redundant code?

client/circuitbreaker/circuit_breaker_test.go

okJiang · 2024-11-29T08:43:12Z

client/circuitbreaker/circuit_breaker_test.go

+		<-ended[i]
+	}
+	assertSucceeds(cb, re)
+	re.Equal(StateClosed, cb.state.stateType)


This check will be failed. Is it expected?

Suggested change

re.Equal(StateClosed, cb.state.stateType)

re.Equal(StateClosed, cb.state.stateType)

re.Equal(uint32(2), cb.state.successCount)

Should we test multi-thread scene more?

This check will be failed. Is it expected?

No, first two requests are counted inside HalfOpen state, then we sent only once request after closing circuit breaker, so successCount is expected to be 1. I'm adding this assert.

Should we test multi-thread scene more?

Multi-threaded testing are hard and verbose, so I tend to add them only to critical functionality like this one. I can add a few more tests to validate that we don't double or undercount, but I feel they are much less critical.

Can you add a case to test two threads call Execute during one state(StateOpen)? Just like this case. I misunderstand them. I think we need them all.

Sure. What invariant I would be testing/asserting on after holding two threads in the Execute?

Just test count. Do not need to assert State.

Can you add a case to test two threads call Execute during one state(StateOpen)?
Open state does not execute calls, but short circuits them, so can't write a pending requests test.

However, I've added TestCircuitBreaker_Count_Only_Requests_In_Same_Window. PTAL.

client/inner_client.go

Signed-off-by: artem_danilov <[email protected]>

client/circuitbreaker/circuit_breaker_test.go

client/circuitbreaker/circuit_breaker.go

nolouch · 2024-12-02T15:44:55Z

client/circuitbreaker/circuit_breaker.go

+	HalfOpenSuccessCount uint32
+}
+
+// AlwaysOpenSettings is a configuration that never trips the circuit breaker.


never trips = AlwaysCloseSettings?

Yes, circuit breaker trips == opens (the current can't flow anymore), meaning never trips == always closed. Or I misunderstood your question? Feel free to suggest a better comment.

So should it be named AlwaysCloseSettings?

Yes, thank you! Fixed.

Signed-off-by: artem_danilov <[email protected]>

Tema · 2024-12-05T05:15:42Z

@rleungx @nolouch @okJiang thanks for review. I think I've addressed most of comments in the last commit:

aligned metric names with state counters
added more documentation to state machine
checked total for 0 and moved ErrorRateThresholdPct > 0 check earlier

Signed-off-by: artem_danilov <[email protected]>

Tema · 2024-12-05T23:41:21Z

/retest

Tema · 2024-12-06T17:24:49Z

TestConnected failed. Does not seem related to this PR. Pass locally.

Tema · 2024-12-06T17:25:26Z

/retest

okJiang

Except #8856 (comment). Rest lgtm

Signed-off-by: artem_danilov <[email protected]>

Tema · 2024-12-10T03:17:40Z

@nolouch @okJiang I've addressed your comments. PTAL.

Also could you please stamp tikv/rfcs#115

okJiang

LGTM, but wait for @nolouch last review

ti-chi-bot · 2024-12-10T03:58:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nolouch, rleungx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nolouch,rleungx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-12-10T03:58:25Z

[LGTM Timeline notifier]

Timeline:

2024-12-09 02:32:50.05079489 +0000 UTC m=+232960.139597433: ☑️ agreed by rleungx.
2024-12-10 03:58:24.664723915 +0000 UTC m=+324494.753526459: ☑️ agreed by nolouch.

introduce circuit breaker for region calls

98312a9

Signed-off-by: artem_danilov <[email protected]>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 26, 2024

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 26, 2024

add default circuit breaker

911cb56

Signed-off-by: artem_danilov <[email protected]>

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 26, 2024

rleungx changed the title ~~[pd-client] introduce circuit breaker for region calls~~ client: introduce circuit breaker for region calls Nov 26, 2024

rleungx requested review from nolouch and okJiang November 26, 2024 06:23

artem_danilov and others added 2 commits November 26, 2024 20:00

fix merge issue interface.go:67:50: undefined: pd

b7678b7

Signed-off-by: artem_danilov <[email protected]>

Merge branch 'master' into circuit-breaker

b993f6b

nolouch requested a review from rleungx November 27, 2024 10:19

rleungx reviewed Nov 28, 2024

View reviewed changes

address PR comments

a95b198

Signed-off-by: artem_danilov <[email protected]>

okJiang reviewed Nov 29, 2024

View reviewed changes

rleungx reviewed Nov 29, 2024

View reviewed changes

client/inner_client.go Show resolved Hide resolved

minor tests and metrics refactoring

6c49334

Signed-off-by: artem_danilov <[email protected]>

rleungx reviewed Dec 2, 2024

View reviewed changes

client/circuitbreaker/circuit_breaker_test.go Show resolved Hide resolved

client/circuitbreaker/circuit_breaker.go Outdated Show resolved Hide resolved

okJiang reviewed Dec 2, 2024

View reviewed changes

client/circuitbreaker/circuit_breaker.go Show resolved Hide resolved

okJiang reviewed Dec 2, 2024

View reviewed changes

client/circuitbreaker/circuit_breaker.go Outdated Show resolved Hide resolved

nolouch reviewed Dec 2, 2024

View reviewed changes

addressed more comments

daffcf6

Signed-off-by: artem_danilov <[email protected]>

Tema and others added 2 commits December 4, 2024 21:16

Merge branch 'master' into circuit-breaker

01a3679

fix merge confilicts issues

2ebfc1e

Signed-off-by: artem_danilov <[email protected]>

rleungx approved these changes Dec 9, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Dec 9, 2024

okJiang reviewed Dec 9, 2024

View reviewed changes

improve testing

45c805e

Signed-off-by: artem_danilov <[email protected]>

okJiang reviewed Dec 10, 2024

View reviewed changes

nolouch approved these changes Dec 10, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 10, 2024

Merge branch 'master' into circuit-breaker

4c0cc9e

ti-chi-bot bot merged commit 1e76110 into tikv:master Dec 10, 2024
25 checks passed

rleungx mentioned this pull request Dec 12, 2024

Implement Circuit Breaker Pattern to Protect PD Leader #8678

Open

	re.Equal(StateClosed, cb.state.stateType)
	re.Equal(StateClosed, cb.state.stateType)
	re.Equal(uint32(2), cb.state.successCount)

client: introduce circuit breaker for region calls #8856

client: introduce circuit breaker for region calls #8856

Conversation

Tema commented Nov 26, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 26, 2024

rleungx commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tema commented Nov 28, 2024

rleungx commented Nov 28, 2024

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

Tema commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tema commented Dec 5, 2024

Tema commented Dec 5, 2024

Tema commented Dec 6, 2024

Tema commented Dec 6, 2024

okJiang left a comment • edited Loading

Choose a reason for hiding this comment

Tema commented Dec 10, 2024

okJiang left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 10, 2024

ti-chi-bot bot commented Dec 10, 2024

[LGTM Timeline notifier]

Tema commented Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

okJiang Dec 2, 2024 •

edited

Loading

okJiang Dec 9, 2024 •

edited

Loading

okJiang left a comment •

edited

Loading