server, client: Deprecate old GC related APIs #9292

MyonKeminta · 2025-05-07T06:01:19Z

What problem does this PR solve?

Issue Number: Close #8978

What is changed and how does it work?

This PR migrates the old GC APIs to the new implementation, and marks them as deprecated.
Also suppresses warnings on necessary invocations to them, such as tests.

Check List

Tests

Unit test

Code changes

-

Side effects

Possible performance regression
Increased code complexity

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

ti-chi-bot · 2025-05-07T06:01:21Z

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

… keyspace is configured (#9468) close #8978 client: Remove public v2 client and redirect call to v2 internally if keyspace is configured Removes the GC V2 related public APIs from PD client to avoid being used by new code. This is a temporary approach, and #9292 will give a complete deprecation. Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2025-07-15T10:21:38Z

/test all

MyonKeminta · 2025-07-15T10:46:50Z

/retest

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…old-gc-api

MyonKeminta · 2025-07-16T11:35:30Z

/test all

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2025-07-17T09:02:17Z

/retest

MyonKeminta · 2025-07-17T09:11:15Z

/retest

MyonKeminta · 2025-07-17T09:23:30Z

/retest

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2025-07-17T10:19:16Z

/retest

MyonKeminta · 2025-07-18T10:59:43Z

/retest

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

rleungx · 2025-07-21T04:19:35Z

server/gc_service.go

 		Header:       wrapHeader(),
 		GcSafePoints: gcSafePoints,
-		Revision:     revision,
+		Revision:     0,


Is the revision not used anymore?

Actually I don't know when it's useful, but now this API cannot provide this field anymore.
cc @ystaticy do you know the purpose of this field?

…old-gc-api

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2025-07-21T07:34:24Z

/retest

MyonKeminta · 2025-07-21T07:49:59Z

/retest

tiancaiamao · 2025-07-21T07:54:18Z

server/api/service_gc_safepoint.go

-	storage := h.svr.GetStorage()
-	gcSafepoint, err := storage.LoadGCSafePoint()
+	gcStateManager := h.svr.GetGCStateManager()
+	gcState, err := gcStateManager.GetGCState(constant.NullKeyspaceID)


Do we have some way to get the Barriers for some keyspace using HTTP API? Or is it necessary?

From a long-term perspective, it's needed, but we haven't done it yet. It should also be integrated into the pd-ctl (see our design document).
Currently, the old service safe point API still works for compatibility, but only for non-keyspace deployment, and does not include gc_worker's service safe point.

ti-chi-bot · 2025-07-21T08:01:52Z

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

rleungx

The rest LGTM

rleungx · 2025-07-22T05:45:32Z

tools/pd-ctl/tests/safepoint/safepoint_test.go

-	re.Equal("Failed to delete service GC safepoint: request pd http api failed with status: '500 Internal Server Error', body: '\"cannot remove service safe point of gc_worker\"'\n", string(output))
+	// This should be rejected previously, but as GC barrier became the replacement of services safe points and we no
+	// longer require the service safe point of "gc_worker", the deletion is now still allowed.
+	// re.Equal("Failed to delete service GC safepoint: request pd http api failed with status: '500 Internal Server Error', body: '\"cannot remove service safe point of gc_worker\"'\n", string(output))


Why not just remove it?

rleungx · 2025-07-22T05:46:11Z

tests/integrations/client/client_test.go


 	// Service safepoint must have a non-empty ID
-	_, err = s.client.UpdateServiceGCSafePoint(context.Background(),
+	//nolint:staticcheck


There are so many comments to disable the lint...

The lint complains about marking it deprecated while keeping it covered by tests 😰
How about wrap it in a closure so that the nolint directive can be limit in fewer places?

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot · 2025-07-23T09:37:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, rleungx, tiancaiamao

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:

~~OWNERS~~ [JmPotato,rleungx]

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 · 2025-07-23T09:37:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, rleungx, tiancaiamao

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:

~~OWNERS~~ [JmPotato,rleungx]

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 · 2025-07-23T09:37:17Z

[LGTM Timeline notifier]

Timeline:

2025-07-23 08:07:17.809625651 +0000 UTC m=+151994.547051846: ☑️ agreed by rleungx.
2025-07-23 09:37:16.648517529 +0000 UTC m=+157393.385943737: ☑️ agreed by JmPotato.

…old-gc-api

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2025-07-24T04:01:44Z

/retest

… keyspace is configured (tikv#9468) close tikv#8978 client: Remove public v2 client and redirect call to v2 internally if keyspace is configured Removes the GC V2 related public APIs from PD client to avoid being used by new code. This is a temporary approach, and tikv#9292 will give a complete deprecation. Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Signed-off-by: okjiang <819421878@qq.com>

close tikv#8978 This PR migrates the old GC APIs to the new implementation, and marks them as deprecated. Also suppresses warnings on necessary invocations to them, such as tests. Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com> Signed-off-by: okjiang <819421878@qq.com>

MyonKeminta changed the title ~~grpc_service, client: Deprecate old GC related APIs~~ server, client: Deprecate old GC related APIs May 7, 2025

MyonKeminta mentioned this pull request May 8, 2025

Refactor GC related PD APIs #8978

Closed

24 tasks

MyonKeminta mentioned this pull request Jun 13, 2025

gc: Add a temporary solution to make service safe point compatible with the new txn safe point mechanism #9407

Merged

MyonKeminta added 4 commits July 10, 2025 18:25

partial commit

0c0a5e8

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Replay changes to tests

afb4071

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

recover updates to the tests for adapted methods

344589c

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Code clean up and redirect implementations (partial)

579bbb5

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/deprecate-old-gc-api branch from 22756d8 to 579bbb5 Compare July 11, 2025 10:02

clean up old code

fc17b04

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta added 2 commits July 16, 2025 19:32

Provide compatibility for V2 APIs and add tests

21a4ad4

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'master' of https://github.com/tikv/pd into m/deprecate-…

ee09992

…old-gc-api

MyonKeminta added 3 commits July 16, 2025 19:41

Fix lint

f006b80

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

fix etcd key test

767c1fb

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

fix etcd key test

2ab0a0d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta marked this pull request as ready for review July 17, 2025 09:23

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 Jul 17, 2025

fix etcd key test

5bfbc69

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

update cleint integration tests

816cc3d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

rleungx reviewed Jul 21, 2025

View reviewed changes

MyonKeminta added 2 commits July 21, 2025 14:27

Merge branch 'master' of https://github.com/tikv/pd into m/deprecate-…

398a77c

…old-gc-api

Fix lint

b00458e

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

tiancaiamao reviewed Jul 21, 2025

View reviewed changes

tiancaiamao approved these changes Jul 21, 2025

View reviewed changes

rleungx reviewed Jul 22, 2025

View reviewed changes

Address comments

704350b

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

rleungx approved these changes Jul 23, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 23, 2025

JmPotato approved these changes Jul 23, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 23, 2025

MyonKeminta added 3 commits July 24, 2025 11:33

Merge branch 'master' of https://github.com/tikv/pd into m/deprecate-…

ba2936e

…old-gc-api

Merge branch 'master' of https://github.com/tikv/pd into m/deprecate-…

aea4e65

…old-gc-api

fix check

1e9e18d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot bot merged commit 0d31172 into tikv:master Jul 24, 2025
33 of 37 checks passed

MyonKeminta deleted the m/deprecate-old-gc-api branch July 24, 2025 10:23

server, client: Deprecate old GC related APIs #9292

server, client: Deprecate old GC related APIs #9292

Uh oh!

Conversation

MyonKeminta commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented May 7, 2025

Uh oh!

MyonKeminta commented Jul 15, 2025

Uh oh!

MyonKeminta commented Jul 15, 2025

Uh oh!

MyonKeminta commented Jul 16, 2025

Uh oh!

MyonKeminta commented Jul 17, 2025

Uh oh!

MyonKeminta commented Jul 17, 2025

Uh oh!

MyonKeminta commented Jul 17, 2025

Uh oh!

MyonKeminta commented Jul 17, 2025

Uh oh!

MyonKeminta commented Jul 18, 2025

Uh oh!

rleungx Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

MyonKeminta Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

MyonKeminta commented Jul 21, 2025

Uh oh!

MyonKeminta commented Jul 21, 2025

Uh oh!

tiancaiamao Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

MyonKeminta Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jul 21, 2025

Uh oh!

rleungx left a comment

Choose a reason for hiding this comment

Uh oh!

rleungx Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

rleungx Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

MyonKeminta Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jul 23, 2025

Uh oh!

ti-chi-bot bot commented Jul 23, 2025

Uh oh!

ti-chi-bot bot commented Jul 23, 2025

[LGTM Timeline notifier]

Uh oh!

MyonKeminta commented Jul 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MyonKeminta commented May 7, 2025 •

edited

Loading

MyonKeminta Jul 22, 2025 •

edited

Loading