Skip to content

Conversation

@tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Issue Number: ref #8978

What is changed and how does it work?

Based on #9340, so please review that PR before this one.
All the global GC barrier related API in the server side are implemented there.

This commit update the pd client part.
And also update GetAllKeyspacesGCStates to return global GC barriers.

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: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
@ti-chi-bot 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 May 27, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented May 27, 2025

Hi @tiancaiamao. 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.

Details

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.

@ti-chi-bot ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 27, 2025
@purelind
Copy link
Contributor

/retest

@tiancaiamao tiancaiamao mentioned this pull request Jun 4, 2025
2 tasks
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 5, 2025
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
@codecov
Copy link

codecov bot commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 72.66187% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.86%. Comparing base (1ac8d8c) to head (11701da).
⚠️ Report is 19 commits behind head on master.

❌ Your patch status has failed because the patch coverage (72.66%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9361      +/-   ##
==========================================
+ Coverage   76.80%   76.86%   +0.05%     
==========================================
  Files         486      486              
  Lines       77396    77525     +129     
==========================================
+ Hits        59444    59589     +145     
+ Misses      14321    14299      -22     
- Partials     3631     3637       +6     
Flag Coverage Δ
unittests 76.86% <72.66%> (+0.05%) ⬆️

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: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
// DeleteGlobalGCBarrier deletes a global GC barrier.
DeleteGlobalGCBarrier(ctx context.Context, barrierID string) (*GlobalGCBarrierInfo, error)
// Get the GC states from all keyspaces.
GetAllKeyspacesGCStates(ctx context.Context) (GCStates, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to include it in this PR, consider adding it to the PR title. Also it might be a good idea to split it into another PR.
Actually I didn't expect this API to be needed in the golang PD client...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no GetGlobalGCBarriers API, so without this, how can you write tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

The GetAllKeyspacesGCStates needs test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Comment on lines +101 to +103
// DeleteGlobalGCBarrier deletes a global GC barrier.
DeleteGlobalGCBarrier(ctx context.Context, barrierID string) (*GlobalGCBarrierInfo, error)
// Get the GC states from all keyspaces.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider also expand these comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'm trying my best already

Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
Signed-off-by: tiancaiamao <tiancaiamao@gmail.com>
@MyonKeminta MyonKeminta mentioned this pull request Oct 9, 2025
24 tasks
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 9, 2025

@MyonKeminta: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

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.

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Oct 9, 2025
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 13, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, MyonKeminta, 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
Contributor

ti-chi-bot bot commented Oct 13, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-10-09 08:04:48.981342658 +0000 UTC m=+341078.012441976: ☑️ agreed by JmPotato.
  • 2025-10-13 02:41:55.226879987 +0000 UTC m=+62021.304132547: ☑️ agreed by rleungx.

@rleungx
Copy link
Member

rleungx commented Oct 13, 2025

/ok-to-test

1 similar comment
@rleungx
Copy link
Member

rleungx commented Oct 13, 2025

/ok-to-test

@ti-chi-bot 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 Oct 13, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 13, 2025

@tiancaiamao: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci/build 09c6e98 link true /test build
non-block/pull-unit-test-next-gen 11701da link false /test pull-unit-test-next-gen

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@ti-chi-bot ti-chi-bot bot merged commit 8e7aea4 into tikv:master Oct 13, 2025
26 of 29 checks passed
JmPotato pushed a commit to JmPotato/pd that referenced this pull request Dec 3, 2025
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 ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants