mcs: register resource manager to etcd #6040

rleungx · 2023-02-23T10:13:28Z

What problem does this PR solve?

This PR is going to register the resource manager into etcd after starting.

Issue Number: Ref #5766.

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-02-23T10:13:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bufferflies
lhy1024

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.

ti-chi-bot · 2023-02-23T10:13:30Z

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 · 2023-02-23T10:23:46Z

Codecov Report

Base: 74.91% // Head: 74.83% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (a0ecabb) compared to base (b2f2a82).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6040      +/-   ##
==========================================
- Coverage   74.91%   74.83%   -0.08%     
==========================================
  Files         367      367              
  Lines       36595    36598       +3     
==========================================
- Hits        27415    27388      -27     
- Misses       6792     6813      +21     
- Partials     2388     2397       +9

Flag	Coverage Δ
unittests	`74.83% <100.00%> (-0.08%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/mcs/resource_manager/server/server.go	`48.75% <100.00%> (+0.97%)`	⬆️
pkg/mcs/discovery/register.go	`53.48% <0.00%> (-18.61%)`	⬇️
pkg/utils/metricutil/metricutil.go	`82.75% <0.00%> (-10.35%)`	⬇️
pkg/member/member.go	`60.00% <0.00%> (-5.79%)`	⬇️
pkg/storage/endpoint/meta.go	`61.36% <0.00%> (-4.55%)`	⬇️
server/region_syncer/server.go	`82.96% <0.00%> (-4.40%)`	⬇️
pkg/storage/storage.go	`67.30% <0.00%> (-3.85%)`	⬇️
pkg/tso/allocator_manager.go	`63.22% <0.00%> (-2.75%)`	⬇️
server/region_syncer/client.go	`86.56% <0.00%> (-1.50%)`	⬇️
pkg/election/lease.go	`84.05% <0.00%> (-1.45%)`	⬇️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lhy1024

the rest LGTM

lhy1024 · 2023-02-23T11:50:30Z

pkg/mcs/resource_manager/server/server.go

@@ -275,6 +279,9 @@ func (s *Server) startServer() error {

 	// Server has started.
 	atomic.StoreInt64(&s.isServing, 1)
+	// register to etcd
+	s.serviceRegister = discovery.NewServiceRegister(s.ctx, s.etcdClient, "resource_manager", s.cfg.ListenAddr, s.cfg.ListenAddr, 3)


how about refactor the function's signature for the ttl unit, and make it more clear.

If more clear, it will be better.

How about using a constant? like leaseInSeconds? I just keep consistent with etcd.

lhy1024 · 2023-02-23T11:51:32Z

pkg/mcs/resource_manager/server/server.go

@@ -275,6 +279,9 @@ func (s *Server) startServer() error {

 	// Server has started.
 	atomic.StoreInt64(&s.isServing, 1)
+	// register to etcd


do we also need to register when running legacy server?

No, it will only register when the resource manager is started as an independent binary.

I think this comment is useless.

I will remove it.

bufferflies · 2023-02-24T03:54:02Z

tests/mcs/resource_manager/server_test.go

+
+	cfg := rm.NewConfig()
+	cfg.BackendEndpoints = leader.GetAddr()
+	cfg.ListenAddr = "127.0.0.1:8087"


What does it happen if the address has been bound? It maybe help

I copied from TestResourceManagerServer, seems it also has the problem. @lhy1024

I will fix it

replace it with tempurl.Alloc() in #6047

bufferflies

reset LGTM

Signed-off-by: Ryan Leung <rleungx@gmail.com>

bufferflies · 2023-02-27T02:31:29Z

/merge

ti-chi-bot · 2023-02-27T02:31:30Z

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

ti-chi-bot · 2023-02-27T02:31:31Z

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

Commit hash: a0ecabb

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

ti-chi-bot requested review from HunDunDM and lhy1024 February 23, 2023 10:13

rleungx marked this pull request as ready for review February 23, 2023 10:13

ti-chi-bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-linked-issue labels Feb 23, 2023

lhy1024 reviewed Feb 23, 2023

View reviewed changes

lhy1024 approved these changes Feb 24, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 24, 2023

bufferflies reviewed Feb 24, 2023

View reviewed changes

rleungx requested a review from bufferflies February 24, 2023 07:17

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

rleungx added 2 commits February 27, 2023 10:24

register resource manager to etcd

74ac74e

Signed-off-by: Ryan Leung <rleungx@gmail.com>

address comments

a0ecabb

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the register-server branch from b7e7ebb to a0ecabb Compare February 27, 2023 02:26

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

bufferflies approved these changes Feb 27, 2023

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 Feb 27, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 27, 2023

ti-chi-bot merged commit 002d6ca into tikv:master Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: register resource manager to etcd #6040

mcs: register resource manager to etcd #6040

rleungx commented Feb 23, 2023 •

edited

Loading

ti-chi-bot commented Feb 23, 2023 •

edited

Loading

ti-chi-bot commented Feb 23, 2023

codecov bot commented Feb 23, 2023 •

edited

Loading

lhy1024 left a comment

lhy1024 Feb 23, 2023

rleungx Feb 24, 2023

bufferflies Feb 24, 2023

lhy1024 Feb 24, 2023

rleungx Feb 24, 2023

lhy1024 Feb 23, 2023

rleungx Feb 24, 2023 •

edited

Loading

bufferflies Feb 24, 2023

rleungx Feb 24, 2023

bufferflies Feb 24, 2023 •

edited

Loading

rleungx Feb 24, 2023

lhy1024 Feb 24, 2023

lhy1024 Feb 24, 2023

bufferflies left a comment

bufferflies commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

mcs: register resource manager to etcd #6040

mcs: register resource manager to etcd #6040

Conversation

rleungx commented Feb 23, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Feb 23, 2023 • edited Loading

ti-chi-bot commented Feb 23, 2023

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

lhy1024 left a 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

rleungx Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bufferflies Feb 24, 2023 • edited Loading

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

bufferflies left a comment

Choose a reason for hiding this comment

bufferflies commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

ti-chi-bot commented Feb 27, 2023

rleungx commented Feb 23, 2023 •

edited

Loading

ti-chi-bot commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

rleungx Feb 24, 2023 •

edited

Loading

bufferflies Feb 24, 2023 •

edited

Loading