Skip to content

feat(router service): support to sync region from pd leader #9845

Merged
ti-chi-bot[bot] merged 7 commits intotikv:masterfrom
bufferflies:sync
Nov 26, 2025
Merged

feat(router service): support to sync region from pd leader #9845
ti-chi-bot[bot] merged 7 commits intotikv:masterfrom
bufferflies:sync

Conversation

@bufferflies
Copy link
Copy Markdown
Contributor

@bufferflies bufferflies commented Oct 17, 2025

What problem does this PR solve?

Issue Number: ref #9212

What is changed and how does it work?

Check List

Tests

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

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

None.

Signed-off-by: 童剑 <1045931706@qq.com>
@ti-chi-bot
Copy link
Copy Markdown
Contributor

ti-chi-bot bot commented Oct 17, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2025
Signed-off-by: 童剑 <1045931706@qq.com>
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 17, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 20, 2025

Codecov Report

❌ Patch coverage is 78.92857% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.52%. Comparing base (d9219d5) to head (c7d1838).
⚠️ Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9845      +/-   ##
==========================================
- Coverage   78.70%   78.52%   -0.18%     
==========================================
  Files         492      503      +11     
  Lines       66268    67131     +863     
==========================================
+ Hits        52153    52717     +564     
- Misses      10408    10592     +184     
- Partials     3707     3822     +115     
Flag Coverage Δ
unittests 78.52% <78.92%> (-0.18%) ⬇️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: 童剑 <1045931706@qq.com>
@bufferflies bufferflies force-pushed the sync branch 2 times, most recently from 23817f0 to c737484 Compare October 23, 2025 04:00
@bufferflies bufferflies marked this pull request as ready for review October 23, 2025 04:34
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2025
@bufferflies bufferflies changed the title feat(router service): support to sync region from pd leader [WIP]feat(router service): support to sync region from pd leader Oct 23, 2025
@ti-chi-bot ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2025
@bufferflies bufferflies force-pushed the sync branch 2 times, most recently from cefb214 to 8e04378 Compare October 23, 2025 07:50
Signed-off-by: 童剑 <1045931706@qq.com>
@bufferflies bufferflies changed the title [WIP]feat(router service): support to sync region from pd leader feat(router service): support to sync region from pd leader Nov 4, 2025
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2025
@@ -138,6 +138,7 @@ func (s *RegionSyncer) RunServer(ctx context.Context, regionNotifier <-chan *cor
select {
case <-ctx.Done():
log.Info("region syncer has been stopped")
s.closeAllClient()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it a leak?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, the follower will recampaign and close the client stream also in most cases.

}
log.Info("server starts to synchronize with leader", zap.String("server", s.name), zap.String("leader", leaderAddr), zap.Uint64("request-index", s.nextSyncIndex))
for {
resp, err := stream.Recv()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to check the io.EOF?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, I think we should handle this error

Copy link
Copy Markdown
Member

@okJiang okJiang 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

s.CloseClientConns()
s.serverLoopCancel()
if s.serverLoopCancel != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seem it cannot be nil

Signed-off-by: 童剑 <1045931706@qq.com>
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 24, 2025
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 25, 2025
@ti-chi-bot
Copy link
Copy Markdown
Contributor

ti-chi-bot bot commented Nov 25, 2025

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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
Copy link
Copy Markdown
Contributor

ti-chi-bot bot commented Nov 25, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-11-24 10:18:12.861297359 +0000 UTC m=+525256.510491815: ☑️ agreed by rleungx.
  • 2025-11-25 08:50:53.898028454 +0000 UTC m=+606417.547222911: ☑️ agreed by okJiang.

@rleungx
Copy link
Copy Markdown
Member

rleungx commented Nov 25, 2025

CI failed

Signed-off-by: 童剑 <1045931706@qq.com>
@bufferflies
Copy link
Copy Markdown
Contributor Author

/test pull-unit-test-next-gen

1 similar comment
@bufferflies
Copy link
Copy Markdown
Contributor Author

/test pull-unit-test-next-gen

@bufferflies
Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@bufferflies
Copy link
Copy Markdown
Contributor Author

/retest

@rleungx
Copy link
Copy Markdown
Member

rleungx commented Nov 26, 2025

/test pull-unit-test-next-gen

@ti-chi-bot ti-chi-bot bot merged commit e2e6ca1 into tikv:master Nov 26, 2025
38 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants