server, endpoint, client: Add PD APIs for RawKV GC Worker #4866

AmoebaProtozoa · 2022-04-27T09:38:02Z

What problem does this PR solve?

Issue Number: ref #4865
RFC: tikv/rfcs#90

What is changed and how does it work?

Added Support for RawKV GC APIs

For more information, please read https://github.com/tikv/rfcs/pull/90

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

Related changes

PR to update kvproto: pd: RawKV GC SafePoint API pingcap/kvproto#908
PR to update tikv/rfc: RFC:TiKV RawKV MVCC GC rfcs#90

ToDo:

Update go mod reference after kvproto has been merged
Unit test & debug
Manual test along with new RawKV GC worker & RawKV CDC tools

Release note

None.

ti-chi-bot · 2022-04-27T09:38:04Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

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

haojinming · 2022-05-06T08:23:32Z

server/storage/endpoint/gc_safe_point.go

@@ -178,11 +194,149 @@ func (se *StorageEndpoint) SaveServiceGCSafePoint(ssp *ServiceSafePoint) error {
 	return se.Save(key, string(value))
 }

-// RemoveServiceGCSafePoint removes a GC safepoint for the service
+// RemoveServiceGCSafePoint removes a GC safeoint for the service


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

haojinming · 2022-05-06T08:35:13Z

server/storage/endpoint/key_path.go

+// gcSafePointPathByServiceGroup returns the path of the gc_worker's safe point
+// /gc_servicegroup/gc_safepoint/$service_group_id
+func gcSafePointPathByServiceGroup(serviceGroupID string) string {
+	return path.Join(gcServiceGroupPath, "gc_safepoint", serviceGroupID)


suggest to define the path including gc_safepoint service_safepoint as const.

…_path.go Signed-off-by: AmoebaProtozoa <[email protected]>

ystaticy · 2022-05-09T01:37:50Z

client/client.go

+		c.ScheduleCheckLeader()
+		return false, 0, errors.WithStack(err)
+	}
+	// if requested safepoint is the new safepoint, then update succeeded


Suggested change

// if requested safepoint is the new safepoint, then update succeeded

// If requested safepoint is the new safepoint, then update succeeded

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

haojinming

LGTM~

ti-chi-bot · 2022-05-10T07:01:31Z

@haojinming: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM~

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 ti-community-infra/tichi repository.

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

pingyu · 2022-05-10T08:36:07Z

/cc @nolouch @MyonKeminta

ti-chi-bot · 2022-05-10T08:36:09Z

@pingyu: GitHub didn't allow me to request PR reviews from the following users: MyonKeminta.

Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @nolouch @MyonKeminta

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.

pingyu · 2022-05-10T08:36:28Z

/cc @nolouch @MyonKeminta

PTAL, thanks~

nolouch · 2022-05-10T09:12:41Z

PTAL @AndreMouche

nolouch

Can you split your PR to small parts to help review?

nolouch · 2022-05-10T09:20:46Z

client/client.go

@@ -130,6 +131,22 @@ type Client interface {
 	UpdateOption(option DynamicOption, value interface{}) error
 	// Close closes the client.
 	Close()
+
+	// GetGCAllServiceGroups returns a list containing all service groups that has safe point in pd
+	GetGCAllServiceGroups(ctx context.Context) ([]string, error)


Better to define independently and then nested in here;

pingyu · 2022-05-10T10:25:32Z

Can you split your PR to small parts to help review?

OK~

pingyu · 2022-05-11T02:05:38Z

Can you split your PR to small parts to help review?

Here is the storage part. PTAL, thanks~

ti-chi-bot · 2022-05-11T11:58:36Z

@AmoebaProtozoa: PR needs rebase.

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 27, 2022

ti-chi-bot requested review from disksing and lhy1024 April 27, 2022 09:38

AmoebaProtozoa force-pushed the RawKV_GC_API branch from 6556954 to c5944e3 Compare April 27, 2022 09:40

AmoebaProtozoa added 7 commits May 5, 2022 14:16

initial implementation of new GC API

ebaa37f

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

rephrased comments

36ba350

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

updated client code

9856c22

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

server side code

bdd1f0e

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

client side code

2c3c1a1

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

update reference

0094a1e

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

updated client and server according to new proto

fd07049

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

AmoebaProtozoa force-pushed the RawKV_GC_API branch from 3160a94 to fd07049 Compare May 5, 2022 06:22

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2022

AmoebaProtozoa added 4 commits May 5, 2022 14:41

changed lock name

2db7af1

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

change server and client to use new gcpb

74d216a

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

update reference, remove go mod replace

911a091

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

added unit tests for storage endpoint

1e682fa

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

AmoebaProtozoa marked this pull request as ready for review May 6, 2022 07:48

AmoebaProtozoa changed the title ~~[WIP] server, endpoint, client: Add PD APIs for RawKV GC Worker~~ server, endpoint, client: Add PD APIs for RawKV GC Worker May 6, 2022

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2022

haojinming reviewed May 6, 2022

View reviewed changes

typo fix

d4d757c

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

haojinming reviewed May 6, 2022

View reviewed changes

add serviceGroupServicePath and serviceGroupGCPath as constant to key…

f67792f

…_path.go Signed-off-by: AmoebaProtozoa <[email protected]>

ystaticy reviewed May 9, 2022

View reviewed changes

pingyu and others added 4 commits May 9, 2022 23:15

bugfix & add integration test

58c4c77

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

fix CI error

3b19687

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

Merge branch 'master' into RawKV_GC_API

fcc34e1

improve integration test & other polish

230c345

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

haojinming approved these changes May 10, 2022

View reviewed changes

polish GC integration test codes

2965b74

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

ti-chi-bot requested a review from nolouch May 10, 2022 08:36

nolouch requested review from AndreMouche and removed request for lhy1024 May 10, 2022 09:12

nolouch reviewed May 10, 2022

View reviewed changes

Merge branch 'master' into RawKV_GC_API

be26bf8

pingyu mentioned this pull request May 11, 2022

storage: Add API for RawKV GC #4918

Closed

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2022

This was referenced May 12, 2022

storage: Add APIs for RawKV GC #4937

Merged

client: Add APIs for RawKV GC #4943

Closed

Add PD APIs for RawKV GC #4865

Closed

AmoebaProtozoa closed this May 13, 2022

AmoebaProtozoa mentioned this pull request May 18, 2022

storage: Use etcd lease to manage safe point expiration #4984

Closed

AmoebaProtozoa mentioned this pull request Jun 9, 2022

server: Add APIs for RawKV GC #5138

Closed

AmoebaProtozoa reopened this Jun 27, 2022

AmoebaProtozoa closed this Mar 23, 2023

AmoebaProtozoa deleted the RawKV_GC_API branch March 23, 2023 02:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server, endpoint, client: Add PD APIs for RawKV GC Worker #4866

server, endpoint, client: Add PD APIs for RawKV GC Worker #4866

AmoebaProtozoa commented Apr 27, 2022 •

edited

Loading

ti-chi-bot commented Apr 27, 2022

haojinming May 6, 2022

haojinming May 6, 2022

ystaticy May 9, 2022

haojinming left a comment

ti-chi-bot commented May 10, 2022

pingyu commented May 10, 2022

ti-chi-bot commented May 10, 2022

pingyu commented May 10, 2022

nolouch commented May 10, 2022

nolouch left a comment

nolouch May 10, 2022 •

edited

Loading

pingyu commented May 10, 2022

pingyu commented May 11, 2022

ti-chi-bot commented May 11, 2022

	// if requested safepoint is the new safepoint, then update succeeded
	// If requested safepoint is the new safepoint, then update succeeded

server, endpoint, client: Add PD APIs for RawKV GC Worker #4866

server, endpoint, client: Add PD APIs for RawKV GC Worker #4866

Conversation

AmoebaProtozoa commented Apr 27, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

ToDo:

Release note

ti-chi-bot commented Apr 27, 2022

haojinming May 6, 2022

Choose a reason for hiding this comment

haojinming May 6, 2022

Choose a reason for hiding this comment

ystaticy May 9, 2022

Choose a reason for hiding this comment

haojinming left a comment

Choose a reason for hiding this comment

ti-chi-bot commented May 10, 2022

pingyu commented May 10, 2022

ti-chi-bot commented May 10, 2022

pingyu commented May 10, 2022

nolouch commented May 10, 2022

nolouch left a comment

Choose a reason for hiding this comment

nolouch May 10, 2022 • edited Loading

Choose a reason for hiding this comment

pingyu commented May 10, 2022

pingyu commented May 11, 2022

ti-chi-bot commented May 11, 2022

AmoebaProtozoa commented Apr 27, 2022 •

edited

Loading

nolouch May 10, 2022 •

edited

Loading