server: Make gc_worker's service safepoint special #3146

MyonKeminta · 2020-11-03T07:48:21Z

Signed-off-by: MyonKeminta [email protected]

What problem does this PR solve?

Fixes #3136

What is changed and how it works?

This PR makes service safepoint with gc_worker a special one:

It has an initial value 0
Its TTL must be math.MaxInt64 (which represents infinity)
It cannot be deleted

Therefore, gc_worker's service safepoint is no longer symmetrical to other services' safepoints.

By the way, this PR also disallowed registering service safepoint with empty ID.

I'm not sure if returning errors.New(...) is a proper way to report errors. If it's not, I need better suggestions.

cc @leoppro

Check List

Tests

Unit test

Code changes

-

Side effects

Increased code complexity

Related changes

Need to cherry-pick to the release branch
- release-4.0

Release note

No release note.

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

MyonKeminta · 2020-11-03T08:10:48Z

/test

overvenus

LGTM

zier-one

LGTM

MyonKeminta · 2020-11-05T04:20:42Z

@rleungx PTAL. Can this be merged?

rleungx · 2020-11-05T06:04:48Z

server/core/storage.go

@@ -45,6 +45,8 @@ const (
 	componentPath            = "component"
 	customScheduleConfigPath = "scheduler_config"
 	encryptionKeysPath       = "encryption_keys"
+	// GCWorkerServiceSafePointID is the ID of GCWorker's service GC safe point.
+	GCWorkerServiceSafePointID = "gc_worker"


How about changing this name to make it private?

It's used in test in tests/client/client_test.go .

prefer to use "gc_worker" directly in tests

server/core/storage.go

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

…nKeminta/pd into m/gcworker-service-safepoint

MyonKeminta · 2020-11-06T04:48:34Z

@rleungx @disksing PTAL again thx

disksing · 2020-11-09T05:36:40Z

/rebase

disksing · 2020-11-09T05:36:57Z

/run-all-tests

disksing · 2020-11-09T05:37:05Z

/merge

ti-chi-bot · 2020-11-09T05:37:05Z

@disksing: It seems you want to merge this PR, I will help you trigger all the tests:
/run-all-tests

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 tidb-community-bots/prow-config repository.

ti-chi-bot · 2020-11-09T05:37:08Z

Can merge label has been added.

Git tree hash: 46a717b

MyonKeminta · 2020-11-09T06:08:59Z

/merge

ti-chi-bot · 2020-11-09T06:09:00Z

@MyonKeminta: It seems you want to merge this PR, I will help you trigger all the tests:
/run-all-tests

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 tidb-community-bots/prow-config repository.

ti-chi-bot · 2020-11-09T06:09:00Z

@MyonKeminta: you cannot merge your own PR.

In response to this:

/merge

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 tidb-community-bots/prow-config repository.

disksing · 2020-11-09T07:27:51Z

/run-integration-lightning-test

MyonKeminta · 2020-11-09T07:29:03Z

CI failed on lightning's test because lightning sets empty string as serviceID, while this PR added non-empty check.

disksing · 2020-11-09T07:39:36Z

CI failed on lightning's test because lightning sets empty string as serviceID, while this PR added non-empty check.

sad story. Should we update the lightning test?

MyonKeminta · 2020-11-09T07:45:27Z

CI failed on lightning's test because lightning sets empty string as serviceID, while this PR added non-empty check.

sad story. Should we update the lightning test?

I thin lightning should fix the bug that it didn't set service ID.

By the way it's lucky we used gc_worker as service ID for TiDB, instead of using empty to show its specialness.

MyonKeminta · 2020-11-09T07:49:18Z

… or perhaps another choice is to remove the empty check from this PR first to merge this PR quickly, and add the empty check in another PR...

Then people who tries to use old versions of lightning on new tidb cluster will get the unfriendly error...
I have no idea...

HunDunDM · 2020-11-09T08:54:05Z

Do we need to add support in pd-recover?

MyonKeminta · 2020-11-09T11:34:19Z

@HunDunDM Sorry I didn't get it. What to recover?

zier-one · 2020-11-10T06:02:05Z

/merge

ti-chi-bot · 2020-11-10T06:02:06Z

@leoppro: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 tidb-community-bots/prow-config repository.

ti-chi-bot · 2020-11-10T06:02:18Z

@leoppro: adding 'status/cam-merge' is restricted to committers in list.

In response to this:

/merge

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 tidb-community-bots/prow-config repository.

zier-one · 2020-11-10T06:02:37Z

/run-all-tests

MyonKeminta · 2020-11-10T06:21:54Z

/run-all-tests

MyonKeminta · 2020-11-10T06:53:46Z

/run-integration-lightning-test tidb-lightning=pr/460

glorv · 2020-11-10T11:45:00Z

/run-all-tests

glorv · 2020-11-11T03:39:11Z

/run-integration-lightning-test

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2020-11-11T03:50:12Z

cherry pick to release-4.0 in PR #3173

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

* server: Make gc_worker's service safepoint special Signed-off-by: MyonKeminta <[email protected]> * Address comments Signed-off-by: MyonKeminta <[email protected]> Co-authored-by: MyonKeminta <[email protected]> Signed-off-by: MyonKeminta <[email protected]>

* server: Make gc_worker's service safepoint special Signed-off-by: MyonKeminta <[email protected]> * Address comments Signed-off-by: MyonKeminta <[email protected]> Co-authored-by: MyonKeminta <[email protected]> Signed-off-by: MyonKeminta <[email protected]> Co-authored-by: MyonKeminta <[email protected]> Co-authored-by: MyonKeminta <[email protected]>

server: Make gc_worker's service safepoint special

1d37799

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

MyonKeminta added the needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. label Nov 3, 2020

MyonKeminta added this to the v4.0.9 milestone Nov 3, 2020

overvenus reviewed Nov 4, 2020

View reviewed changes

Merge branch 'master' into m/gcworker-service-safepoint

445e9f3

zier-one reviewed Nov 5, 2020

View reviewed changes

rleungx reviewed Nov 5, 2020

View reviewed changes

disksing reviewed Nov 5, 2020

View reviewed changes

server/core/storage.go Show resolved Hide resolved

MyonKeminta added 2 commits November 5, 2020 18:44

Address comments

34e6c15

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

Merge branch 'm/gcworker-service-safepoint' of https://github.com/Myo…

46a717b

…nKeminta/pd into m/gcworker-service-safepoint

rleungx approved these changes Nov 6, 2020

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 6, 2020

disksing approved these changes Nov 9, 2020

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 9, 2020

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 9, 2020

MyonKeminta mentioned this pull request Nov 9, 2020

Lightning sets empty string as service ID for UpdateServiceGCSafePoint API pingcap/tidb-lightning#458

Closed

Merge branch 'master' into m/gcworker-service-safepoint

2ccfbb5

MyonKeminta mentioned this pull request Nov 10, 2020

checksum: fix pd service id is empty pingcap/tidb-lightning#460

Merged

ti-chi-bot merged commit a140b42 into tikv:master Nov 11, 2020

ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Nov 11, 2020

cherry pick tikv#3146 to release-4.0

dfff967

Signed-off-by: ti-srebot <[email protected]>

ti-srebot mentioned this pull request Nov 11, 2020

server: Make gc_worker's service safepoint special (#3146) #3173

Merged

MyonKeminta deleted the m/gcworker-service-safepoint branch November 11, 2020 04:06

MyonKeminta added a commit to ti-srebot/pd that referenced this pull request Nov 11, 2020

cherry pick tikv#3146 to release-4.0

47aedb9

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

This was referenced Jan 19, 2021

Service Safepoint with infinite TTL may keep disappearing after upgrading PD #3366

Closed

server: Auto fix gc_worker's service safepoint for upgraded clusters #3371

Merged

This was referenced Jan 26, 2021

server: Auto fix gc_worker's service safepoint for upgraded clusters (#3371) #3391

Merged

server: Auto fix gc_worker's service safepoint for upgraded clusters (#3371) #3392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: Make gc_worker's service safepoint special #3146

server: Make gc_worker's service safepoint special #3146

MyonKeminta commented Nov 3, 2020

MyonKeminta commented Nov 3, 2020

overvenus left a comment

zier-one left a comment

MyonKeminta commented Nov 5, 2020

rleungx Nov 5, 2020

MyonKeminta Nov 5, 2020

rleungx Nov 5, 2020

MyonKeminta commented Nov 6, 2020

disksing commented Nov 9, 2020

disksing commented Nov 9, 2020

disksing commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

disksing commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

disksing commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020 •

edited

Loading

HunDunDM commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

zier-one commented Nov 10, 2020

ti-chi-bot commented Nov 10, 2020

ti-chi-bot commented Nov 10, 2020

zier-one commented Nov 10, 2020

MyonKeminta commented Nov 10, 2020

MyonKeminta commented Nov 10, 2020

glorv commented Nov 10, 2020

glorv commented Nov 11, 2020

ti-srebot commented Nov 11, 2020

server: Make gc_worker's service safepoint special #3146

server: Make gc_worker's service safepoint special #3146

Conversation

MyonKeminta commented Nov 3, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

MyonKeminta commented Nov 3, 2020

overvenus left a comment

Choose a reason for hiding this comment

zier-one left a comment

Choose a reason for hiding this comment

MyonKeminta commented Nov 5, 2020

rleungx Nov 5, 2020

Choose a reason for hiding this comment

MyonKeminta Nov 5, 2020

Choose a reason for hiding this comment

rleungx Nov 5, 2020

Choose a reason for hiding this comment

MyonKeminta commented Nov 6, 2020

disksing commented Nov 9, 2020

disksing commented Nov 9, 2020

disksing commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

ti-chi-bot commented Nov 9, 2020

disksing commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

disksing commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020 • edited Loading

HunDunDM commented Nov 9, 2020

MyonKeminta commented Nov 9, 2020

zier-one commented Nov 10, 2020

ti-chi-bot commented Nov 10, 2020

ti-chi-bot commented Nov 10, 2020

zier-one commented Nov 10, 2020

MyonKeminta commented Nov 10, 2020

MyonKeminta commented Nov 10, 2020

glorv commented Nov 10, 2020

glorv commented Nov 11, 2020

ti-srebot commented Nov 11, 2020

MyonKeminta commented Nov 9, 2020 •

edited

Loading