gc: Implement compatibility methods and `GetAllKeyspacesGCStates` method to `GCStateManager` #9134

MyonKeminta · 2025-03-11T07:57:43Z

What problem does this PR solve?

Issue Number: Ref #8978

What is changed and how does it work?

Implement compatibility methods and `GetAllKeyspacesGCStates` method to `GCStateManager`, which is planned to be the replacement of `SafePointManager`.

Blocked by:

~~storage/endpoint: Add a new component GCStateStorage and GCStateProvider to storage/endpoints as the new storage layer for GC related metadata #9109~~
gc: Add GCStateManager which is planned to be the replacement of SafePointManager #9169

This PR is branched from #9109 which is not merged yet. If you want to view the diff in this PR without being disturbed by changes from #9109, please see: MyonKeminta/pd@m/gc-state-storage...MyonKeminta:pd:m/gc-state-manager

Part of this PR is split out to #9169 . Please merge #9169 first. To see changes in this PR based on #9169, see: MyonKeminta/pd@m/gc-state-manager-partial...MyonKeminta:pd:m/gc-state-manager

Check List

Tests

Unit test

Code changes

Side effects

Possible performance regression
Increased code complexity

Related changes

Release note

None.

Signed-off-by: MyonKeminta <[email protected]>

…torage

Signed-off-by: MyonKeminta <[email protected]>

ti-chi-bot · 2025-03-11T07:57:46Z

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

codecov · 2025-03-11T08:04:21Z

Codecov Report

Attention: Patch coverage is 77.85235% with 33 lines in your changes missing coverage. Please review.

Project coverage is 75.79%. Comparing base (9092ccc) to head (e978f22).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9134      +/-   ##
==========================================
+ Coverage   75.72%   75.79%   +0.06%     
==========================================
  Files         477      477              
  Lines       73437    73584     +147     
==========================================
+ Hits        55612    55773     +161     
+ Misses      14342    14331      -11     
+ Partials     3483     3480       -3

Flag	Coverage Δ
unittests	`75.79% <77.85%> (+0.06%)`	⬆️

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: MyonKeminta <[email protected]>

…torage

Signed-off-by: MyonKeminta <[email protected]>

ti-chi-bot · 2025-04-09T07:51:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~OWNERS~~ [JmPotato]

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-04-09T07:51:27Z

[LGTM Timeline notifier]

Timeline:

2025-04-09 07:51:27.572139062 +0000 UTC m=+2242781.256375158: ☑️ agreed by JmPotato.

Signed-off-by: MyonKeminta <[email protected]>

rleungx · 2025-04-09T10:08:18Z

pkg/gc/gc_state_manager.go

@@ -194,9 +218,23 @@ func (m *GCStateManager) advanceGCSafePointImpl(keyspaceID uint32, target uint64
 		return wb.SetGCSafePoint(keyspaceID, target)
 	})
 	if err != nil {
+		log.Error("failed to advance GC safe point",
+			zap.Uint32("keyspace-idD", keyspaceID),


Suggested change

zap.Uint32("keyspace-idD", keyspaceID),

zap.Uint32("keyspace-id", keyspaceID),

rleungx · 2025-04-09T10:13:10Z

pkg/gc/gc_state_manager.go

+
+// saturatingDuration returns a duration calculated by multiplying the given `ratio` and `base`, truncated within the
+// range [0, math.MaxInt64] to avoid negative value and overflowing.
+func saturatingDuration(ratio int64, base time.Duration) time.Duration {


Why do we need this function?

It's used for avoiding overflow after multiplication, so that it prevents the user from specifying a too large value and causing undefined behavior.

Btw I renamed it to saturatingMultiplyDuration

Will it cover up mistakes? BTW, we have enabled durationcheck linter.

Signed-off-by: MyonKeminta <[email protected]>

rleungx · 2025-04-10T07:41:49Z

pkg/gc/gc_state_manager.go

+		}
+		if res.NewTxnSafePoint != newServiceSafePoint {
+			minServiceSafePoint = &endpoint.ServiceSafePoint{
+				ServiceID: "__pseudo_service:" + res.BlockerDescription,


Is it possible to unify the format of ServiceID? __pseudo_service: TiDBMinStartTS { Key: %+q, MinStartTS: %d } looks wired.

Do you have any suggestion?

MyonKeminta · 2025-04-10T10:02:51Z

/hold
Some details still need discussion.

MyonKeminta added 19 commits February 27, 2025 17:07

Add chagnes to the path definition

c70bd8e

Signed-off-by: MyonKeminta <[email protected]>

Add gc_states.go and gc_states_test.go

84b09c4

Signed-off-by: MyonKeminta <[email protected]>

fix test

70c0ce9

Signed-off-by: MyonKeminta <[email protected]>

Add more comments

ba36b14

Signed-off-by: MyonKeminta <[email protected]>

fix lint

26fc18c

Signed-off-by: MyonKeminta <[email protected]>

Add more comments

2622255

Signed-off-by: MyonKeminta <[email protected]>

Remove unnecessary commented code

db9fa7b

Signed-off-by: MyonKeminta <[email protected]>

Add more comments

364cf6d

Signed-off-by: MyonKeminta <[email protected]>

Address comments

c5653a2

Signed-off-by: MyonKeminta <[email protected]>

Update file headers

2f0c663

Signed-off-by: MyonKeminta <[email protected]>

Renaming

c1731dd

Signed-off-by: MyonKeminta <[email protected]>

Merge path functions

d9b944f

Signed-off-by: MyonKeminta <[email protected]>

rename keypath/safe_point.go to gc_states.go

f1647bf

Signed-off-by: MyonKeminta <[email protected]>

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

b913e1e

…torage

Add comments explaining keyspace-level GC and unified GC

11a002e

Signed-off-by: MyonKeminta <[email protected]>

Address comments

8e4bd77

Signed-off-by: MyonKeminta <[email protected]>

Address comments

0a5acfd

Signed-off-by: MyonKeminta <[email protected]>

fix lint

42cae00

Signed-off-by: MyonKeminta <[email protected]>

Add related files

632a99d

Signed-off-by: MyonKeminta <[email protected]>

MyonKeminta changed the title ~~gc: Add GCStateManager which is planned to be the replacement of SafePointManager.~~ gc: Add GCStateManager which is planned to be the replacement of SafePointManager Mar 11, 2025

MyonKeminta mentioned this pull request Mar 11, 2025

Refactor GC related PD APIs #8978

Open

17 tasks

MyonKeminta added 3 commits March 12, 2025 11:05

Address comments

397a53f

Signed-off-by: MyonKeminta <[email protected]>

Add test for GCBarrer.IsExpired

6445fa4

Signed-off-by: MyonKeminta <[email protected]>

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

106639f

…torage

MyonKeminta and others added 14 commits March 31, 2025 13:59

Address comments

66cf8e9

Signed-off-by: MyonKeminta <[email protected]>

Remove saturatingDuration

051eb23

Signed-off-by: MyonKeminta <[email protected]>

Rename keyspace GC management type configuration values

cc2c527

Signed-off-by: MyonKeminta <[email protected]>

Add comments

4841da6

Signed-off-by: MyonKeminta <[email protected]>

Address partial of the comments

355d092

Signed-off-by: MyonKeminta <[email protected]>

Address comments

369726b

Signed-off-by: MyonKeminta <[email protected]>

Move log printing into a separated function

8553c0a

Signed-off-by: MyonKeminta <[email protected]>

fix cases in logs

51e8561

Signed-off-by: MyonKeminta <[email protected]>

fix lint

63747ef

Signed-off-by: MyonKeminta <[email protected]>

fix lint

885e1fc

Signed-off-by: MyonKeminta <[email protected]>

Merge branch 'master' into m/gc-state-manager-partial

fbcfcb8

Merge branch 'm/gc-state-manager-partial' into m/gc-state-manager

9ef2a3b

fix renaming

f3a4b06

Signed-off-by: MyonKeminta <[email protected]>

Merge remote-tracking branch 'upstream/master' into m/gc-state-manager

3e41130

MyonKeminta marked this pull request as ready for review April 8, 2025 06:06

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 Apr 8, 2025

fix etcd key test

dcb1f84

Signed-off-by: MyonKeminta <[email protected]>

JmPotato approved these changes Apr 9, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 9, 2025

ti-chi-bot bot added the approved label Apr 9, 2025

refine logs

cc88bc8

Signed-off-by: MyonKeminta <[email protected]>

rleungx reviewed Apr 9, 2025

View reviewed changes

MyonKeminta added 2 commits April 10, 2025 12:08

fix lint; address comments; renaming

aa80488

Signed-off-by: MyonKeminta <[email protected]>

Update comments

e978f22

Signed-off-by: MyonKeminta <[email protected]>

MyonKeminta mentioned this pull request Apr 10, 2025

server: Add gRPC interfaces for the new GC APIs. #9189

Draft

rleungx reviewed Apr 10, 2025

View reviewed changes

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gc: Implement compatibility methods and `GetAllKeyspacesGCStates` method to `GCStateManager` #9134

gc: Implement compatibility methods and `GetAllKeyspacesGCStates` method to `GCStateManager` #9134

MyonKeminta commented Mar 11, 2025 •

edited

Loading

ti-chi-bot bot commented Mar 11, 2025

codecov bot commented Mar 11, 2025 •

edited

Loading

ti-chi-bot bot commented Apr 9, 2025

ti-chi-bot bot commented Apr 9, 2025

rleungx Apr 9, 2025

rleungx Apr 9, 2025

MyonKeminta Apr 10, 2025

MyonKeminta Apr 10, 2025

rleungx Apr 10, 2025

rleungx Apr 10, 2025

MyonKeminta Apr 10, 2025

MyonKeminta commented Apr 10, 2025

	zap.Uint32("keyspace-idD", keyspaceID),
	zap.Uint32("keyspace-id", keyspaceID),

gc: Implement compatibility methods and GetAllKeyspacesGCStates method to GCStateManager #9134

Are you sure you want to change the base?

gc: Implement compatibility methods and GetAllKeyspacesGCStates method to GCStateManager #9134

Conversation

MyonKeminta commented Mar 11, 2025 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Mar 11, 2025

codecov bot commented Mar 11, 2025 • edited Loading

Codecov Report

ti-chi-bot bot commented Apr 9, 2025

ti-chi-bot bot commented Apr 9, 2025

[LGTM Timeline notifier]

rleungx Apr 9, 2025

Choose a reason for hiding this comment

rleungx Apr 9, 2025

Choose a reason for hiding this comment

MyonKeminta Apr 10, 2025

Choose a reason for hiding this comment

MyonKeminta Apr 10, 2025

Choose a reason for hiding this comment

rleungx Apr 10, 2025

Choose a reason for hiding this comment

rleungx Apr 10, 2025

Choose a reason for hiding this comment

MyonKeminta Apr 10, 2025

Choose a reason for hiding this comment

MyonKeminta commented Apr 10, 2025

gc: Implement compatibility methods and `GetAllKeyspacesGCStates` method to `GCStateManager` #9134

gc: Implement compatibility methods and `GetAllKeyspacesGCStates` method to `GCStateManager` #9134

MyonKeminta commented Mar 11, 2025 •

edited

Loading

codecov bot commented Mar 11, 2025 •

edited

Loading