Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SCB-1039]Add interface to compatible with ServiceCenter Aggregator #1007

Merged
merged 3 commits into from Nov 26, 2018
Merged

[SCB-1039]Add interface to compatible with ServiceCenter Aggregator #1007

merged 3 commits into from Nov 26, 2018

Conversation

heyile
Copy link
Contributor

@heyile heyile commented Nov 24, 2018

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@@ -216,7 +216,7 @@ public static boolean updateInstanceProperties(Map<String, String> instancePrope
}

public static Microservice getMicroservice(String microserviceId) {
return serviceRegistry.getRemoteMicroservice(microserviceId);
return serviceRegistry.getAggregatedRemoteMicroervice(microserviceId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not change this

@@ -58,6 +58,15 @@ MicroserviceInstances findServiceInstances(String appId, String microserviceName

Microservice getRemoteMicroservice(String microserviceId);

/**
* <p>
* if connect to simple ServiceCenter, same with the method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normal, not "sample"?

@@ -353,6 +358,16 @@ public String getSchema(String microserviceId, String schemaId) {
return microservice.getSchemaMap().get(schemaId);
}

@Override
public String getAggregatedSchema(String microserviceId, String schemaId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not copy code
just invoke getSchema

@@ -188,6 +188,11 @@ public Microservice getMicroservice(String microserviceId) {
return microserviceIdMap.get(microserviceId);
}

@Override
public Microservice getAggregatedMicroservice(String microserviceId) {
return microserviceIdMap.get(microserviceId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not copy code
just invoke getMicroservice


@Override
public String getAggregatedSchema(String microserviceId, String schemaId) {
Holder<GetSchemaResponse> holder = new Holder<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not copy code
just make global to be a parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -489,7 +534,7 @@ public String registerMicroserviceInstance(MicroserviceInstance instance) {
CountDownLatch countDownLatch = new CountDownLatch(1);
RestUtils.get(ipPort,
String.format(Const.REGISTRY_API.MICROSERVICE_INSTANCE_OPERATION_ALL, providerId),
new RequestParam().addHeader("X-ConsumerId", consumerId),
new RequestParam().addHeader("X-ConsumerId", consumerId).addQueryParam("global", "true"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to add global

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@coveralls
Copy link

coveralls commented Nov 24, 2018

Coverage Status

Coverage increased (+0.01%) to 86.782% when pulling 31ffdf7 on heyile:aggregated into ca4dea2 on apache:master.

@@ -795,7 +841,7 @@ public ServiceCenterInfo getServiceCenterInfo() {
CountDownLatch countDownLatch = new CountDownLatch(1);
RestUtils.get(ipPort,
Const.REGISTRY_API.SERVICECENTER_VERSION,
new RequestParam(),
new RequestParam().addQueryParam("global", "true"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to add global

return doGetSchema(microserviceId, schemaId, false);
}

private String doGetSchema(String microserviceId, String schemaId, boolean isAggregatedServiceCenter) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not "isAggregatedServiceCenter"
target is ServiceCenter Aggregator:
1.for register flow, global=false
2.for find instance flow, global=true
so the variable name better to be: global

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i will change it

@liubao68
Copy link
Contributor

Why we need to distinguish from register & find flow? I think service center knows the request is register or find.

@heyile
Copy link
Contributor Author

heyile commented Nov 26, 2018

Why we need to distinguish from register & find flow? I think service center knows the request is register or find.

@wujimin I do not know the details very clearly

@wujimin
Copy link
Contributor

wujimin commented Nov 26, 2018

both register and find instance flow, need to invoke getMicroservice/getSchema
but:
only local serviceCenter cluster available for register flow
all serviceCenter clusters in aggregator available for find instance flow

ServiceCenter can not know the scenes, so need client invoke with the flag

@liubao68 liubao68 merged commit 880091d into apache:master Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants