Skip to content
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

kv-client: add stream binding background region worker, not enabled by default #1397

Merged
merged 26 commits into from
Feb 24, 2021

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented Feb 3, 2021

What problem does this PR solve?

Preparatory work for #1393

What is changed and how it works?

  1. Add a new configuration item in changefeed config, kv-client-v2, default false now, which will help us test the new feature. After refactor is stable, this config can be removed. (NOTE: add or remove a new item in changefeed config is both forward compatible and backward compatible) Use a hard code const variable kvClientV2, which is simpler
  2. Keep original single routine event feed processing logic in cdc/kv/client.go
  3. Add a new region worker implement in cdc/kv/region_worker.go, the region worker is single goroutine now, and has a state lock for all regions that it manages. One gRPC stream <-> One region worker (multiple routines support in another pr) <-> One resolve lock routine
  4. Separate lock resolve logic from single region logic

NOTE:

  • New logic keeps the same processing logic as much as possible against the each region one routine way, the unit test cases can be reused. I have tested that the client v2 can pass all integration tests and unit tests.
  • Both step-3 and step-4 will be optimizated in next PR, this PR is reviewable and can be merged first.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note

@amyangfei amyangfei added the DNM label Feb 3, 2021
@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei amyangfei added DNM and removed DNM labels Feb 3, 2021
@amyangfei amyangfei closed this Feb 3, 2021
@amyangfei amyangfei reopened this Feb 3, 2021
@amyangfei amyangfei force-pushed the kv-client-reduce-goroutine-2 branch from 19f00ff to ab83d81 Compare February 4, 2021 08:38
@codecov-io
Copy link

codecov-io commented Feb 4, 2021

Codecov Report

Merging #1397 (dba81bf) into master (2e2b19d) will decrease coverage by 1.1008%.
The diff coverage is 80.1587%.

@@               Coverage Diff                @@
##             master      #1397        +/-   ##
================================================
- Coverage   48.6903%   47.5895%   -1.1009%     
================================================
  Files           138        140         +2     
  Lines         13668      14043       +375     
================================================
+ Hits           6655       6683        +28     
- Misses         6317       6662       +345     
- Partials        696        698         +2     

@amyangfei amyangfei force-pushed the kv-client-reduce-goroutine-2 branch from bb6aeb8 to 9d7ce1c Compare February 5, 2021 07:19
@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei amyangfei changed the title kv-client: reduce goroutine, test only, DNM kv-client: add stream binding background region worker, not enabled by default Feb 18, 2021
@amyangfei amyangfei added status/ptal Could you please take a look? needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. and removed DNM labels Feb 18, 2021
@amyangfei amyangfei added this to the v4.0.12 milestone Feb 18, 2021
pkg/config/config.go Outdated Show resolved Hide resolved
cdc/kv/client_test.go Outdated Show resolved Hide resolved
@amyangfei
Copy link
Contributor Author

PTAL @leoppro @liuzix

@amyangfei
Copy link
Contributor Author

/run-all-tests

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

@@ -375,13 +375,14 @@ func (w *regionWorker) evictAllRegions(ctx context.Context) error {
w.statesLock.Lock()
defer w.statesLock.Unlock()
for _, state := range w.regionStates {
log.Info("on region fail fired", zap.Reflect("state", state))
state.lock.RLock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the following, in case onRegionFail blocks.

		state.lock.RLock()
		singleRegionInfo := state.sri
		state.lock.RUnlock()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onRegionFail is non-blocking way
Besides, this code will be rewritten in later PR

@overvenus
Copy link
Member

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 24, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 24, 2021
@amyangfei
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 24, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@amyangfei merge failed.

@amyangfei
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-integration-tests

@ti-srebot
Copy link
Contributor

@amyangfei merge failed.

@amyangfei
Copy link
Contributor Author

[2021-02-24T13:43:34.622Z] Post https://coveralls.io/api/v1/jobs: stream error: stream ID 1; PROTOCOL_ERROR
[2021-02-24T13:43:34.622Z] make: *** [coverage] Error 1
script returned exit code 2

@amyangfei
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 5c4503c into pingcap:master Feb 24, 2021
@amyangfei amyangfei deleted the kv-client-reduce-goroutine-2 branch February 24, 2021 15:55
@amyangfei amyangfei mentioned this pull request Feb 25, 2021
12 tasks
@amyangfei
Copy link
Contributor Author

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Feb 25, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/kv-client TiKV kv log client component. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. status/ptal Could you please take a look?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants