pkg: support service discovery #5911

rleungx · 2023-02-06T08:26:37Z

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

What problem does this PR solve?

This PR supports service discovery for microservice architecture. The detailed usage can be found in the unit tests.

Issue Number: Ref #5895.

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None

ti-chi-bot · 2023-02-06T08:26:39Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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-06T08:26:39Z

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-06T08:34:25Z

Codecov Report

Base: 75.38% // Head: 75.23% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (a563136) compared to base (0b9395d).
Patch coverage: 59.25% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5911      +/-   ##
==========================================
- Coverage   75.38%   75.23%   -0.15%     
==========================================
  Files         359      362       +3     
  Lines       36167    36221      +54     
==========================================
- Hits        27265    27252      -13     
- Misses       6547     6600      +53     
- Partials     2355     2369      +14

Flag	Coverage Δ
unittests	`75.23% <59.25%> (-0.15%)`	⬇️

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

Impacted Files	Coverage Δ
server/schedule/operator/builder.go	`86.60% <ø> (+0.32%)`	⬆️
pkg/mcs/discovery/register.go	`53.48% <53.48%> (ø)`
pkg/mcs/discovery/discover.go	`77.77% <77.77%> (ø)`
pkg/mcs/discovery/key_path.go	`100.00% <100.00%> (ø)`
pkg/utils/tempurl/tempurl.go	`45.00% <0.00%> (-25.00%)`	⬇️
...erver/config/service_middleware_persist_options.go	`91.66% <0.00%> (-8.34%)`	⬇️
pkg/dashboard/adapter/manager.go	`79.31% <0.00%> (-6.90%)`	⬇️
pkg/tso/local_allocator.go	`71.62% <0.00%> (-6.76%)`	⬇️
pkg/tso/allocator_manager.go	`63.22% <0.00%> (-5.65%)`	⬇️
pkg/election/leadership.go	`75.25% <0.00%> (-2.07%)`	⬇️
... 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.

rleungx · 2023-02-06T08:36:58Z

@binshi-bing PTAL

binshi-bing · 2023-02-06T17:16:41Z

server/schedule/operator/builder.go

@@ -524,7 +524,7 @@ func (b *Builder) prepareBuild() (string, error) {
 	}

 	// Although switch witness may have nothing to do with conf change (except switch witness voter to non-witness voter:


binshi-bing · 2023-02-06T17:23:56Z

pkg/mcs/discovery/register.go

+		cancel: cancel,
+		cli:    cli,
+		key:    serviceKey,
+		value:  serviceAddr,


Since we use range scan to discover, can we use "/pd/microserivce/{service name}/registry/{ip:port}" as the key, weight or a json, which contains host properties and starts with weight, as the value?

For more details of my comment, please see my comments in the design doc.

I'm going to change the value to a string, which can be serialized by different services themselves.

sounds good

binshi-bing · 2023-02-06T17:44:55Z

pkg/mcs/discovery/discover.go

+)
+
+// Discover is used to get all the service instances of the specified service name.
+func Discover(cli *clientv3.Client, serviceName string) ([]string, error) {


The design doc contains server register, service instance discovery and service discovery. This function is the second one. For tenant tso, in the third part "service discover", we need to get the server endpoint for a given key space group. Could you list which functionalities are provided by this PR? And we can implement the remaining functionalities in the next PRs.

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

binshi-bing

LGTM, though it isn't counted

ti-chi-bot · 2023-02-07T18:15:34Z

@binshi-bing: 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, though it isn't counted

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.

lhy1024

the rest LGTM

lhy1024 · 2023-02-08T11:10:22Z

pkg/mcs/discovery/key_path.go

+	registryKey    = "registry"
+)
+
+func registryPath(serviceName, serviceAddr string) string {


do we need tenant now?

service instance (node) registry path doesn't contain tenant/keyspace. The tenant/keyspace group primary discovery path does.

rleungx · 2023-02-09T10:21:40Z

/merge

ti-chi-bot · 2023-02-09T10:21:41Z

@rleungx: 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-09T10:21:42Z

@rleungx: /merge in this pull request requires 2 approval(s).

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

rleungx · 2023-02-09T10:22:23Z

/merge

ti-chi-bot · 2023-02-09T10:22:25Z

@rleungx: 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-09T10:22:28Z

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

Commit hash: 1067533

ti-chi-bot · 2023-02-09T10:22:39Z

@rleungx: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 6, 2023

ti-chi-bot requested review from HunDunDM and nolouch February 6, 2023 08:26

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-linked-issue labels Feb 6, 2023

rleungx marked this pull request as ready for review February 6, 2023 08:35

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

rleungx requested review from JmPotato and lhy1024 February 6, 2023 08:36

binshi-bing reviewed Feb 6, 2023

View reviewed changes

rleungx added 2 commits February 7, 2023 09:59

support service discovery

769c426

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

address comments

1067533

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

rleungx force-pushed the service-discovery branch from 8c3baca to 1067533 Compare February 7, 2023 08:16

binshi-bing approved these changes Feb 7, 2023

View reviewed changes

binshi-bing mentioned this pull request Feb 8, 2023

Separate TSO into an independent service #5836

Closed

lhy1024 reviewed Feb 8, 2023

View reviewed changes

rleungx requested a review from lhy1024 February 9, 2023 04:50

lhy1024 approved these changes Feb 9, 2023

View reviewed changes

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

rleungx added the require-LGT1 Indicates that the PR requires an LGTM. label Feb 9, 2023

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

Merge branch 'master' into service-discovery

a563136

ti-chi-bot merged commit 2be26ff into tikv:master Feb 9, 2023

rleungx deleted the service-discovery branch February 10, 2023 02:04

rleungx mentioned this pull request Mar 3, 2023

Provide API service interfaces #5849

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg: support service discovery #5911

pkg: support service discovery #5911

rleungx commented Feb 6, 2023 •

edited

Loading

ti-chi-bot commented Feb 6, 2023 •

edited

Loading

ti-chi-bot commented Feb 6, 2023

codecov bot commented Feb 6, 2023 •

edited

Loading

rleungx commented Feb 6, 2023

binshi-bing Feb 6, 2023

binshi-bing Feb 6, 2023

binshi-bing Feb 6, 2023

rleungx Feb 7, 2023 •

edited

Loading

binshi-bing Feb 7, 2023

binshi-bing Feb 6, 2023

binshi-bing left a comment

ti-chi-bot commented Feb 7, 2023

lhy1024 left a comment

lhy1024 Feb 8, 2023

binshi-bing Feb 8, 2023

rleungx commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

rleungx commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

		@@ -524,7 +524,7 @@ func (b *Builder) prepareBuild() (string, error) {
		}

		// Although switch witness may have nothing to do with conf change (except switch witness voter to non-witness voter:

pkg: support service discovery #5911

pkg: support service discovery #5911

Conversation

rleungx commented Feb 6, 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 6, 2023 • edited Loading

ti-chi-bot commented Feb 6, 2023

codecov bot commented Feb 6, 2023 • edited Loading

Codecov Report

rleungx commented Feb 6, 2023

binshi-bing Feb 6, 2023

Choose a reason for hiding this comment

binshi-bing Feb 6, 2023

Choose a reason for hiding this comment

binshi-bing Feb 6, 2023

Choose a reason for hiding this comment

rleungx Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

binshi-bing Feb 7, 2023

Choose a reason for hiding this comment

binshi-bing Feb 6, 2023

Choose a reason for hiding this comment

binshi-bing left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 7, 2023

lhy1024 left a comment

Choose a reason for hiding this comment

lhy1024 Feb 8, 2023

Choose a reason for hiding this comment

binshi-bing Feb 8, 2023

Choose a reason for hiding this comment

rleungx commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

rleungx commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

ti-chi-bot commented Feb 9, 2023

rleungx commented Feb 6, 2023 •

edited

Loading

ti-chi-bot commented Feb 6, 2023 •

edited

Loading

codecov bot commented Feb 6, 2023 •

edited

Loading

rleungx Feb 7, 2023 •

edited

Loading