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) #1455

Merged

Conversation

ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Feb 25, 2021

cherry-pick #1397 to release-4.0
You can switch your code base to this Pull Request by using git-extras:

# In ticdc repo:
git pr https://github.com/pingcap/ticdc/pull/1455

After apply modifications, you can push your change to this PR via:

git push git@github.com:ti-srebot/ticdc.git pr/1455:release-4.0-5c4503cf90e9

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

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor

I will resolve conflicts later

@amyangfei amyangfei changed the base branch from release-4.0 to release-4.0-pending February 25, 2021 13:50
@amyangfei amyangfei force-pushed the release-4.0-5c4503cf90e9 branch from 6b156e4 to f673880 Compare February 26, 2021 05:53
@amyangfei
Copy link
Contributor

/run-all-tests

@amyangfei
Copy link
Contributor

/run-integration-tests

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 26, 2021
@amyangfei amyangfei modified the milestones: v4.0.11, v4.0.12 Feb 26, 2021
@amyangfei
Copy link
Contributor

/merge

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

/run-all-tests

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (release-4.0-pending@adef1a9). Click here to learn what that means.
The diff coverage is n/a.

@@                   Coverage Diff                    @@
##             release-4.0-pending      #1455   +/-   ##
========================================================
  Coverage                       ?   47.6432%           
========================================================
  Files                          ?        142           
  Lines                          ?      14193           
  Branches                       ?          0           
========================================================
  Hits                           ?       6762           
  Misses                         ?       6723           
  Partials                       ?        708           

@ti-srebot ti-srebot merged commit db4f1dd into pingcap:release-4.0-pending Feb 26, 2021
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. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. status/ptal Could you please take a look?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants