id: make id allocator general purpose by AmoebaProtozoa · Pull Request #5284 · tikv/pd

AmoebaProtozoa · 2022-07-07T07:56:29Z

Signed-off-by: David 8039876+AmoebaProtozoa@users.noreply.github.com

What problem does this PR solve?

The current implementation of id allocator permits only one id allocator due to fixed alloc path $rootPath/alloc_id and metrics label.
This PR attempts to make it more general purpose by parametrize its alloc path and metrics label.
Issue Number: Close #5294

What is changed and how does it work?

make id allocator general purpose

Check List

Tests

Unit tests

Release note

None.

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

ti-chi-bot · 2022-07-07T07:56:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

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.

Details

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

codecov · 2022-07-07T08:04:24Z

Codecov Report

Merging #5284 (9e82cfb) into master (f5d4636) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5284      +/-   ##
==========================================
+ Coverage   75.70%   75.84%   +0.13%     
==========================================
  Files         311      312       +1     
  Lines       30826    30978     +152     
==========================================
+ Hits        23338    23496     +158     
- Misses       5485     5487       +2     
+ Partials     2003     1995       -8

Flag	Coverage Δ
unittests	`75.84% <100.00%> (+0.13%)`	⬆️

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

Impacted Files	Coverage Δ
server/id/metrics.go	`100.00% <ø> (ø)`
server/id/id.go	`84.61% <100.00%> (+3.66%)`	⬆️
server/server.go	`75.36% <100.00%> (+1.22%)`	⬆️
server/schedulers/shuffle_region_config.go	`63.15% <0.00%> (-7.90%)`	⬇️
pkg/dashboard/adapter/manager.go	`79.31% <0.00%> (-6.90%)`	⬇️
server/election/leadership.go	`75.25% <0.00%> (-2.07%)`	⬇️
server/member/member.go	`64.21% <0.00%> (-1.58%)`	⬇️
server/region_syncer/client.go	`85.82% <0.00%> (-0.75%)`	⬇️
server/tso/tso.go	`71.75% <0.00%> (-0.57%)`	⬇️
server/tso/allocator_manager.go	`65.22% <0.00%> (-0.50%)`	⬇️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5d4636...9e82cfb. Read the comment docs.

rleungx · 2022-07-07T08:08:04Z

Can you describe more details of your use case?

AmoebaProtozoa · 2022-07-07T08:16:37Z

Can you describe more details of your use case?

This is mostly for the Keyspace management in PD.
Keyspace needs a new id everytime a keyspace is created, something very similar to what id allocator does. It would be great if we can reuse id allocator's logic.
However, since it need to be stored on a different storage path and impose some extra restrictions, using the same id allocator instance doesn't seems possible.

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

ystaticy · 2022-07-14T03:16:18Z

LGTM

ti-chi-bot · 2022-07-14T03:17:39Z

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

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

rleungx

The rest LGTM.

rleungx · 2022-07-14T03:58:20Z

+	metrics   *metrics
+	member    string


please reorder these two fields

rleungx · 2022-07-14T04:02:49Z


-	log.Info("idAllocator allocates a new id", zap.Uint64("alloc-id", end))
-	idallocGauge.Set(float64(end))
+	log.Info("idAllocator allocates a new id", zap.String("label", alloc.label), zap.Uint64("alloc-id", end))


The docs of the PD recovery process may need to be updated.

Curiously, is there anything broken so we have to update PD recovery process?

Curiously, is there anything broken so we have to update PD recovery process?

It's this block related to finding the maximum allocated ID
https://github.com/pingcap/docs/blob/master/pd-recover.md#get-allocated-id-from-pd-log

Once we recover PD, do we also need to recover all id allocators?

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

sunxiaoguang · 2022-07-17T12:58:53Z

+}
+
+// metrics is a collection of idAllocator's metrics
+type metrics struct {


Why we need to have this type instead of using prometheus.Guage directly?

It was to improve the readability and make it easier to add more metrics, as suggested #5284 (comment)

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

nolouch

lgtm

nolouch · 2022-07-18T03:02:18Z

ptal @sunxiaoguang

nolouch · 2022-07-20T09:34:52Z

/merge

ti-chi-bot · 2022-07-20T09:34:53Z

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

/run-all-tests

Details

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.

ti-chi-bot · 2022-07-20T09:34:55Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 9e82cfb

close tikv#5294 make id allocator general purpose Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

close #5898 The old version of idalloc is a globally unique path, and keyspace needs to use id allocator to allocate keyspaceID, so in this pr the id allocator is changed to a general purpose, but not sync metrics. #5284 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: JmPotato <ghzpotato@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

…6016) close #5898, ref #5899 The old version of idalloc is a globally unique path, and keyspace needs to use id allocator to allocate keyspaceID, so in this pr the id allocator is changed to a general purpose, but not sync metrics. #5284 Signed-off-by: husharp <jinhao.hu@pingcap.com> Co-authored-by: husharp <jinhao.hu@pingcap.com>

make id allocator more general purporse

5369493

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 7, 2022

ti-chi-bot requested review from disksing and rleungx July 7, 2022 07:56

AmoebaProtozoa closed this Jul 7, 2022

AmoebaProtozoa reopened this Jul 7, 2022

ystaticy reviewed Jul 7, 2022

View reviewed changes

Comment thread server/id/id.go Outdated

ystaticy reviewed Jul 10, 2022

View reviewed changes

Comment thread server/server.go Outdated

ystaticy reviewed Jul 10, 2022

View reviewed changes

Comment thread server/id/id.go Outdated

AmoebaProtozoa added 2 commits July 11, 2022 10:23

use constant for default path and label

7b0a242

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

added unit tests for multiple allocators

22a2b5d

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

ti-chi-bot removed the do-not-merge/needs-linked-issue label Jul 11, 2022

ystaticy reviewed Jul 11, 2022

View reviewed changes

Comment thread server/id/id_test.go Outdated

ystaticy reviewed Jul 11, 2022

View reviewed changes

Comment thread server/id/id_test.go Outdated

update comments

6a0d82b

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

sunxiaoguang reviewed Jul 13, 2022

View reviewed changes

Comment thread server/id/id.go Outdated

nolouch reviewed Jul 13, 2022

View reviewed changes

Comment thread server/id/id.go Outdated

AmoebaProtozoa added 2 commits July 13, 2022 15:41

warp idGauge in a metrics collection

4c2ec9f

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

Merge branch 'master' into id_allocator

bed1e89

AmoebaProtozoa marked this pull request as ready for review July 14, 2022 03:17

AmoebaProtozoa changed the title ~~[WIP] id: make id allocator general purpose~~ id: make id allocator general purpose Jul 14, 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 Jul 14, 2022

ystaticy approved these changes Jul 14, 2022

View reviewed changes

Comment thread server/id/id_test.go Outdated

Merge branch 'master' into id_allocator

5d24d57

rleungx approved these changes Jul 14, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 14, 2022

AmoebaProtozoa added 2 commits July 14, 2022 12:16

reorganize id allocator fields

84730e5

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

typo fix

06f7cbd

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

sunxiaoguang reviewed Jul 17, 2022

View reviewed changes

Comment thread server/id/id.go Outdated

parametrized allocation step, added AllocatorParams

9e82cfb

Signed-off-by: David <8039876+AmoebaProtozoa@users.noreply.github.com>

nolouch approved these changes Jul 18, 2022

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 Jul 18, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 20, 2022

Merge branch 'master' into id_allocator

6026a4c

ti-chi-bot merged commit a7023b0 into tikv:master Jul 20, 2022

AmoebaProtozoa deleted the id_allocator branch August 7, 2022 06:05

Conversation

AmoebaProtozoa commented Jul 7, 2022 • 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 commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rleungx commented Jul 7, 2022

Uh oh!

AmoebaProtozoa commented Jul 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ystaticy commented Jul 14, 2022

Uh oh!

Uh oh!

ti-chi-bot commented Jul 14, 2022

Uh oh!

rleungx left a comment

Choose a reason for hiding this comment

Uh oh!

rleungx Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

rleungx Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

sunxiaoguang Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

AmoebaProtozoa Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

rleungx Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

sunxiaoguang Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

AmoebaProtozoa Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nolouch left a comment

Choose a reason for hiding this comment

Uh oh!

nolouch commented Jul 18, 2022

Uh oh!

nolouch commented Jul 20, 2022

Uh oh!

ti-chi-bot commented Jul 20, 2022

Uh oh!

ti-chi-bot commented Jul 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AmoebaProtozoa commented Jul 7, 2022 •

edited

Loading

ti-chi-bot commented Jul 7, 2022 •

edited

Loading

codecov Bot commented Jul 7, 2022 •

edited

Loading