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-1971] Introduce SPI mechanism to enable/disable zero-config and … #1804

Merged
merged 5 commits into from Jun 2, 2020
Merged

Conversation

jungan21
Copy link
Contributor

@jungan21 jungan21 commented Jun 1, 2020

…changes to adapt to new interfaces

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 -Pit 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.

@coveralls
Copy link

coveralls commented Jun 1, 2020

Coverage Status

Coverage decreased (-0.09%) to 86.46% when pulling 3c8088a on jungan21:master into 9a0c45f on apache:master.

public class ZeroConfigDiscovery implements Discovery {

private static final String NAME = "zero-config discovery";
private static final String ENABLED = "servicecomb.zeroconfig.registry.discovery.enabled";
Copy link
Contributor

Choose a reason for hiding this comment

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

For zero config, Discovery and Registration must be enabled/disable togather. So it's better to use servicecomb.zeroconfig.registry.enabled. I'll modify other modules later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will just define this variable once in the Constant class/interface

@Override
public String getSchema(String microserviceId, Collection<MicroserviceInstance> instances,
String schemaId) {
return zeroConfigClient.getSchema(microserviceId, schemaId);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can return null in future. Loading schema from instance will the core process, and this change is in progress, so can keep this code here for temporary usage. Maybe add a TODO is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wil add // TODO comment

private MulticastSocket multicastSocket;

// Constructor
public ZeroConfigClient(ZeroConfigRegistryService zeroConfigRegistryService,
Copy link
Contributor

Choose a reason for hiding this comment

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

Constructor is better to be private for singletan. And add a method with @VisibleForTesting to create new ZeroConfigClient instance.

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


// setter/getter

public void setSelfMicroservice(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe @VisibleForTesting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree


private String doRegister(Map<String, String> serviceInstanceDataMap) {
try {
byte[] instanceData = serviceInstanceDataMap.toString().getBytes();
Copy link
Contributor

Choose a reason for hiding this comment

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

specify locale to avoid problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

specify locale to avoid problems.

? please explain more

//throw new ServiceCombException("Failed to create MulticastSocket object", e);
}
return new ZeroConfigClient(new ZeroConfigRegistryService(), multicastSocket,
new RestTemplate());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can not use RestTemlate here bacause it is can only invoke simple services that do not deploy any security features, e.g. when TLS is eanbled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will think about perfect soution in next PR

@liubao68 liubao68 merged commit e0d13ea into apache:master Jun 2, 2020
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

3 participants