storage/endpoint: Add a new component `GCStateStorage` and `GCStateProvider` to storage/endpoints as the new storage layer for GC related metadata #9109

MyonKeminta · 2025-02-27T10:16:21Z

What problem does this PR solve?

Issue Number: Ref #8978

What is changed and how does it work?

Add a new component `GCStateStorage` and `GCStateProvider` to storage/endpoints as the new storage layer for GC related metadata.
This is part of the GC Interface Refactor task. The `GCSafePointStorage` is planned to be deprecated, and the GCStateStorage will be the replacement. It's currently not used, and will be used when we refactor the upper layer (`GCSafePointManager`->`GCStateManager`, the gRPC layer, etc.).

Check List

Tests

Unit test

Code changes

Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)

Side effects

Possible performance regression
Increased code complexity

Related changes

Release note

None.

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

ti-chi-bot · 2025-02-27T10:16:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign nolouch for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

codecov · 2025-02-27T10:48:10Z

Codecov Report

Attention: Patch coverage is 79.68338% with 77 lines in your changes missing coverage. Please review.

Project coverage is 76.37%. Comparing base (027c0d7) to head (c5653a2).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9109      +/-   ##
==========================================
+ Coverage   76.29%   76.37%   +0.07%     
==========================================
  Files         473      474       +1     
  Lines       71663    72034     +371     
==========================================
+ Hits        54677    55016     +339     
- Misses      13559    13578      +19     
- Partials     3427     3440      +13

Flag	Coverage Δ
unittests	`76.37% <79.68%> (+0.07%)`	⬆️

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

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

MyonKeminta · 2025-02-28T06:03:59Z

/retest

rleungx · 2025-02-28T08:55:10Z

pkg/storage/endpoint/gc_states.go

+	"github.com/tikv/pd/pkg/utils/keypath"
+)
+
+// ServiceSafePoint is the safepoint for a specific service


How about adding more comments to explain which component or feature will use ServiceSafePoint/GCBarrier/TxnSafePoint?

I'll add comments to the start of this file to describe the related concepts. But ths interfaces in this file is only expected to be used in GCStateManager, which will come in another PR.

rleungx · 2025-02-28T09:04:30Z

pkg/storage/endpoint/gc_states.go

+
+// keyspaceGCSafePoint is the data structure when writing
+type keyspaceGCSafePoint struct {
+	KeyspaceID uint32 `json:"keyspace_id"`


What is the ID used for?

Nothing but for keeping the data format compatible with the exisiting design of the "GC API V2". I forgot to finish the comments of this type. I'll fix it.

rleungx · 2025-02-28T09:06:35Z

pkg/storage/endpoint/gc_states.go

+}
+
+// loadJSON loads a specific key from the StorageEndpoint, and parses it as JSON into type T.
+func loadJSON[T any](se *StorageEndpoint, key string) (T, error) {


Shall we move loadJSON and loadJSONByPrefix to endpoint.go?

rleungx · 2025-03-03T03:28:34Z

pkg/storage/endpoint/gc_states.go

+// In the transaction, reads can be performed on the GCStateProvider as usual, while writes should only be performed
+// through the GCStateWriteBatch.
+func (p GCStateProvider) RunInGCStateTransaction(f func(wb *GCStateWriteBatch) error) error {
+	revisionKey := keypath.GCStateRevisionPath()


Is it possible to use the revision provided by etcd?

Yes but hard to provide it over the abstracted kv.Base interface.

And it needs a single centrialized key anyway.

rleungx · 2025-03-03T03:32:49Z

pkg/storage/endpoint/gc_states.go

+	txn := p.storage.CreateRawTxn()
+	result, err := txn.If(condition).Then(ops...).Commit()
+	if err != nil {
+		return errors.AddStack(err)


How about return errs.ErrEtcdTxnInternal.Wrap(err).GenWithStackByArgs()

rleungx · 2025-03-03T03:33:30Z

pkg/storage/endpoint/gc_states.go

+		return errs.ErrEtcdTxnConflict.GenWithStackByArgs()
+	}
+
+	if len(ops) != len(result.Responses) {


If this happens, do we need to rollback or manually fix it?

If I understanding is correct, etcd shoudl guarantee this to be true. I expect that this path is never reachable. If this happen, I think it should be a bug in etcd or etcd-client and it doesn't look possible to automatically fix it in a simple way. And as the Commit returns no error here, it seems we no longer have chance to roll it back atomically. I think if this does happen, we may have no choice but to manually recover it.

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

MyonKeminta · 2025-03-03T06:23:56Z

/retest

MyonKeminta · 2025-03-03T06:31:45Z

/retest

ystaticy · 2025-03-03T10:16:31Z

pkg/storage/endpoint/gc_states.go

@@ -0,0 +1,541 @@
+// Copyright 2024 TiKV Project Authors.


Suggested change

// Copyright 2024 TiKV Project Authors.

// Copyright 2025 TiKV Project Authors.

Time went too fast before I noticed it😕

ystaticy · 2025-03-04T07:53:02Z

pkg/utils/keypath/absolute_key_path.go

-	gcSafePointV2PathFormat      = "/pd/%d/keyspaces/gc_safe_point/%08d"         // "/pd/{cluster_id}/keyspaces/gc_safe_point/{keyspace_id}"
-	serviceSafePointV2PathFormat = "/pd/%d/keyspaces/service_safe_point/%08d/%s" // "/pd/{cluster_id}/keyspaces/service_safe_point/{keyspace_id}/{service_id}"
+	gcStateRevisionPathFormat     = "/pd/%d/gc/gc_state_revision"         // "/pd/{cluster_id}/gc/gc_state_revision"
+	gcSafePointPathFormat         = "/pd/%d/gc/safe_point"                // "/pd/{cluster_id}/gc/safe_point"


How about naming this variable globalGCSafePointPathFormat directly? Other developers might have questions about the relationship between gcSafePointPathFormat and keyspaceGCSafePointPathFormat: keyspaceGCSafePointPathFormat.

ystaticy · 2025-03-04T07:58:52Z

pkg/utils/keypath/safe_point.go

+	return fmt.Sprintf(gcStateRevisionPathFormat, ClusterID())
+}
+
+// GCSafePointPath returns the key path of the global (NullKeyspace) GC safe point.


How about explicitly introducing and explaining the relationship between global and NullKeyspace in the code comments? This can help everyone understand its meaning clearly.

MyonKeminta added 2 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]>

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. labels Feb 27, 2025

ti-chi-bot bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 27, 2025

MyonKeminta mentioned this pull request Feb 27, 2025

Refactor GC related PD APIs #8978

Open

13 tasks

fix test

70c0ce9

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

MyonKeminta added 4 commits February 27, 2025 19:31

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]>

rleungx reviewed Feb 28, 2025

View reviewed changes

rleungx reviewed Mar 3, 2025

View reviewed changes

MyonKeminta added 2 commits March 3, 2025 13:38

Add more comments

364cf6d

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

Address comments

c5653a2

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

ystaticy reviewed Mar 3, 2025

View reviewed changes

ystaticy reviewed Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage/endpoint: Add a new component `GCStateStorage` and `GCStateProvider` to storage/endpoints as the new storage layer for GC related metadata #9109

storage/endpoint: Add a new component `GCStateStorage` and `GCStateProvider` to storage/endpoints as the new storage layer for GC related metadata #9109

MyonKeminta commented Feb 27, 2025 •

edited

Loading

ti-chi-bot bot commented Feb 27, 2025

codecov bot commented Feb 27, 2025 •

edited

Loading

MyonKeminta commented Feb 28, 2025

rleungx Feb 28, 2025

MyonKeminta Mar 3, 2025

rleungx Feb 28, 2025

MyonKeminta Feb 28, 2025

rleungx Feb 28, 2025

rleungx Mar 3, 2025

MyonKeminta Mar 3, 2025

MyonKeminta Mar 3, 2025

rleungx Mar 3, 2025

rleungx Mar 3, 2025

MyonKeminta Mar 3, 2025

MyonKeminta commented Mar 3, 2025

MyonKeminta commented Mar 3, 2025

ystaticy Mar 3, 2025

MyonKeminta Mar 4, 2025

ystaticy Mar 4, 2025

ystaticy Mar 4, 2025

	// Copyright 2024 TiKV Project Authors.
	// Copyright 2025 TiKV Project Authors.

storage/endpoint: Add a new component GCStateStorage and GCStateProvider to storage/endpoints as the new storage layer for GC related metadata #9109

Are you sure you want to change the base?

storage/endpoint: Add a new component GCStateStorage and GCStateProvider to storage/endpoints as the new storage layer for GC related metadata #9109

Conversation

MyonKeminta commented Feb 27, 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 Feb 27, 2025

codecov bot commented Feb 27, 2025 • edited Loading

Codecov Report

MyonKeminta commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MyonKeminta commented Mar 3, 2025

MyonKeminta commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storage/endpoint: Add a new component `GCStateStorage` and `GCStateProvider` to storage/endpoints as the new storage layer for GC related metadata #9109

storage/endpoint: Add a new component `GCStateStorage` and `GCStateProvider` to storage/endpoints as the new storage layer for GC related metadata #9109

MyonKeminta commented Feb 27, 2025 •

edited

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading