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.

Sign up for GitHub

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

Jump to bottom

ctl: replace gc_safepoint call with PD HTTP SDK #8504

Merged
merged 36 commits into from
Aug 21, 2024
Merged

ctl: replace gc_safepoint call with PD HTTP SDK #8504

merged 36 commits into from
Aug 21, 2024

Conversation

JackL9u
Copy link
Contributor

@JackL9u JackL9u commented Aug 7, 2024 •
edited by rleungx
Loading

What problem does this PR solve?

Issue Number: Ref #7300

What is changed and how does it work?

replace gc_safepoint call with PD HTTP SDK

Release note

None.

@JackL9u
gc_safepoint HTTP SDK
3d65bc8 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
fix an indentation mismatch
c307b89 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Aug 7, 2024
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Aug 7, 2024

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

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/test-infra repository.

@ti-chi-bot ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 7, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.59%. Comparing base (ce1d0e8) to head (7965d10).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8504      +/-   ##
==========================================
+ Coverage   77.53%   77.59%   +0.05%     
==========================================
  Files         474      474              
  Lines       61864    61877      +13     
==========================================
+ Hits        47967    48012      +45     
+ Misses      10361    10331      -30     
+ Partials     3536     3534       -2
Flag Coverage Δ
unittests 77.59% <93.33%> (+0.05%) ⬆️

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

JmPotato
JmPotato reviewed Aug 7, 2024
View reviewed changes
Copy link
Member

@JmPotato JmPotato left a comment

Choose a reason for hiding this comment

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

Overall LGTM, please add one or more tests.

client/http/api.go Outdated Show resolved Hide resolved
client/http/interface.go Outdated Show resolved Hide resolved
@rleungx
Copy link
Member

rleungx commented Aug 8, 2024

/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 Aug 8, 2024
rleungx
rleungx reviewed Aug 8, 2024
View reviewed changes
client/http/interface.go Outdated Show resolved Hide resolved
client/http/interface.go Outdated Show resolved Hide resolved
tools/pd-ctl/pdctl/command/gc_safepoint_command.go Outdated Show resolved Hide resolved
client/http/interface.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot bot added dco-signoff: no Indicates the PR's author has not signed dco. and removed dco-signoff: yes Indicates the PR's author has signed the dco. labels Aug 8, 2024
@JackL9u
fix some coding style issues
637c50c 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. and removed dco-signoff: no Indicates the PR's author has not signed dco. labels Aug 8, 2024
@JackL9u
format the code
002862c 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
fix a var-name issue
419b903 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
add test functions
6ac2bc2 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
JmPotato
JmPotato reviewed Aug 8, 2024
View reviewed changes
client/http/interface.go Outdated Show resolved Hide resolved
@JackL9u
add test functions for gc safepoint call
8a8a40f 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 8, 2024
@JackL9u
fix coding style for testing suite
cc78799 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
fix coding style for testing suite again
c88639b 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
changed the test case for get SafePoint
870f9cc 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
edit test function
be5c4a1 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 15, 2024
@JackL9u
added tests
98b09b7 
Signed-off-by: Boyang Lyu <boyangly@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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 15, 2024
@JackL9u
add the delete gc_worker check
785e975 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
add a comment
f522d45 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
modified the delete gc_worker test
6f4d633 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u
run the tests again
e3517c8 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JackL9u JackL9u force-pushed the dev1 branch 2 times, most recently from 3c1af75 to 604784e Compare August 16, 2024 07:59
@JackL9u
add a comment
d25953b 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@JmPotato
Copy link
Member

@rleungx Please help take a look, thanks.

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Aug 19, 2024
HuSharp
HuSharp reviewed Aug 20, 2024
View reviewed changes
client/http/types.go Show resolved Hide resolved
client/http/types.go Show resolved Hide resolved
server/api/service_gc_safepoint.go Show resolved Hide resolved
tools/pd-ctl/pdctl/command/gc_safepoint_command.go Show resolved Hide resolved
tools/pd-ctl/tests/safepoint/safepoint_test.go Outdated Show resolved Hide resolved
tests/integrations/client/http_client_test.go Outdated Show resolved Hide resolved
@JackL9u
add delete non-exist safepoints tests
dded6c8 
Signed-off-by: Boyang Lyu <boyangly@gmail.com>
@ti-chi-bot ti-chi-bot bot added the lgtm label Aug 21, 2024
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Aug 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HuSharp, JmPotato

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

The pull request process is described here

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 ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 21, 2024
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Aug 21, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-08-19 03:08:36.472985061 +0000 UTC m=+148511.607435183: ☑️ agreed by JmPotato.
  • 2024-08-21 06:18:33.956239367 +0000 UTC m=+332709.090689490: ☑️ agreed by HuSharp.

@JackL9u
Copy link
Contributor Author

JackL9u commented Aug 21, 2024

/retest

@ti-chi-bot ti-chi-bot bot merged commit 6c30bbf into tikv:master Aug 21, 2024
21 checks passed
@JackL9u JackL9u deleted the dev1 branch August 21, 2024 07:46
@HuSharp HuSharp mentioned this pull request Aug 28, 2024
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rleungx rleungx rleungx left review comments

@HuSharp HuSharp HuSharp approved these changes

@JmPotato JmPotato JmPotato approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JackL9u @rleungx @JmPotato @HuSharp