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

client: Implement batch client #308

Merged
merged 29 commits into from Mar 12, 2020
Merged

Conversation

MyonKeminta
Copy link
Contributor

@MyonKeminta MyonKeminta commented Mar 4, 2020

What problem does this PR solve?

This PR implements batch client to reuse one stream for multiple regions.
Fixes #292
Also fixes #303

What is changed and how it works?

This PR switches kvproto to https://github.com/5kbpers/kvproto/tree/cdc-batch-service . For each invoking to EventFeed, at most one stream will be connected to each TiKV instance.

There will be one sender goroutine that dispatches requests to each stream, one receiver goroutine for each stream receives the events from it. However, different from the original design, there are still one goroutine for each region. The receiver goroutines doesn't process the events by themselves, instead, they dispatches the events to the goroutines for each region. It's because to maintain the region's states in a single goroutine is complicated, and to make it work as soon as possible, currently these goroutines are still kept. I'll continue working on further refactoring and optimizing.

In this PR, error handling still needs polishing.

Check List

Tests

  • Integration test
  • Manual test (add detailed scripts or steps below)

Side effects

  • Increased code complexity
  • Breaking backward compatibility

@amyangfei amyangfei mentioned this pull request Mar 6, 2020
@MyonKeminta MyonKeminta changed the title [WIP] client: Implement batch client client: Implement batch client Mar 11, 2020
@amyangfei amyangfei added component/kv-client TiKV kv log client component. priority/p0 Issue with the highest priority, should be resolved as soon as possible. status/ptal Could you please take a look? labels Mar 11, 2020
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Copy link
Contributor

@5kbpers 5kbpers left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

LGTM

@zier-one zier-one added LGT2 and removed status/ptal Could you please take a look? labels Mar 11, 2020
cdc/kv/client.go Outdated Show resolved Hide resolved
cdc/kv/client.go Show resolved Hide resolved
MyonKeminta and others added 2 commits March 11, 2020 23:53
Co-Authored-By: amyangfei <amyangfei@gmail.com>
@zier-one
Copy link
Contributor

/run-all-tests

2 similar comments
@zier-one
Copy link
Contributor

/run-all-tests

@zier-one
Copy link
Contributor

/run-all-tests

@zier-one zier-one merged commit abd20f5 into pingcap:master Mar 12, 2020
@codecov-io
Copy link

Codecov Report

Merging #308 into master will increase coverage by 1.29%.
The diff coverage is 0.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   28.39%   29.69%   +1.29%     
==========================================
  Files          59       55       -4     
  Lines        5558     5261     -297     
==========================================
- Hits         1578     1562      -16     
+ Misses       3843     3560     -283     
- Partials      137      139       +2
Impacted Files Coverage Δ
cdc/kv/client.go 13.13% <0.29%> (-6.05%) ⬇️
cdc/roles/manager.go 67.87% <0%> (-4.85%) ⬇️
cdc/processor.go 0% <0%> (ø) ⬆️
cdc/sink/sink.go 0% <0%> (ø) ⬆️
cdc/sink/mq.go
cdc/model/sink.go
cdc/model/mq.go
cdc/sink/black_hole.go
cdc/entry/mounter.go 2.58% <0%> (+0.1%) ⬆️
cdc/sink/mysql.go 8.01% <0%> (+0.92%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13e1d52...45f3151. Read the comment docs.

@MyonKeminta MyonKeminta deleted the m/batch-client branch March 12, 2020 04:39
5kbpers pushed a commit to 5kbpers/ticdc that referenced this pull request Aug 24, 2020
amyangfei pushed a commit to amyangfei/tiflow that referenced this pull request May 6, 2022
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. priority/p0 Issue with the highest priority, should be resolved as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EventFeed may miss some regions when region is splitting Support batch stream in kv client
5 participants