[JAV-435] starter to register spring cloud applications to service center #361

acsukesh · 2017-12-06T13:21:31Z

…nter

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 [JAV-XXX] Fixes bug in ApproximateQuantiles, where you replace JAV-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.

…nter

coveralls · 2017-12-06T13:39:16Z

Coverage decreased (-0.03%) to 86.907% when pulling 6ccb2ff on acsukesh:master into b162b31 on ServiceComb:master.

WillemJiang · 2017-12-06T22:25:14Z

spring-boot-starter/spring-boot-starter-registry/pom.xml

+		</dependency>
+		<dependency>
+			<groupId>org.springframework</groupId>
+			<artifactId>spring-web</artifactId>


Do we need to have this dependency?

Yes. To extend RestTemplate we use httpclient apis from this.

we can remove dependency to handler-loadbalance and spring-web and not need to extend RestTemplate

deleted RestTemplate

wujimin · 2017-12-07T03:14:27Z

...er-registry/src/main/java/io/servicecomb/springboot/starter/registry/RegistryIntializer.java

+      try {
+        RegistryUtils.init();
+        RegistryUtils.run();
+        RegistryUtils.getMicroserviceInstance().getEndpoints().add(RegistryUtils.getPublishAddress("rest", address));


endpoint should be setted before registry run

…nter

coveralls · 2017-12-07T07:42:38Z

Coverage decreased (-0.04%) to 86.895% when pulling e1b51fb on acsukesh:master into b162b31 on ServiceComb:master.

liubao68 · 2017-12-07T10:35:49Z

...-registry/src/main/java/io/servicecomb/springboot/starter/registry/CseAutoConfiguration.java

+  }
+
+  @Bean
+  public SpringCloudTransport getTransport() {


This code can be removed if possible

liubao68 · 2017-12-07T10:36:39Z

...gistry/src/main/java/io/servicecomb/springboot/starter/registry/SpringCloudRestTemplate.java

@@ -0,0 +1,155 @@
+/*


We can remove this class if possible

liubao68 · 2017-12-07T10:36:54Z

...-registry/src/main/java/io/servicecomb/springboot/starter/registry/SpringCloudTransport.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2017 Huawei Technologies Co., Ltd


We can remove this class if possible

liubao68 · 2017-12-07T10:37:30Z

...ry/src/test/java/io/servicecomb/springboot/starter/registry/TestSpringCloudRestTemplate.java

@@ -0,0 +1,150 @@
+


We can remove this class if possible

liubao68 · 2017-12-07T10:37:38Z

...istry/src/test/java/io/servicecomb/springboot/starter/registry/TestSpringCloudTransport.java

@@ -0,0 +1,48 @@
+/*


We can remove this class if possible

…nter

liubao68 · 2017-12-07T11:03:10Z

...istry/src/test/java/io/servicecomb/springboot/starter/registry/TestCseAutoConfiguration.java

+
+  @Test
+  public void testGetTransport() {
+    Assert.assertNotNull(cseAutoConfiguration);


It's better to add an assert to test endpoint is the expected one and to add new test case to test when rest address is not properly set.

liubao68 · 2017-12-07T11:03:17Z

...egistry/src/test/java/io/servicecomb/springboot/starter/registry/TestRegistryIntializer.java

+  }
+
+  @Test
+  public void testInitRegistry1() {


I think this test case is covered by TestCseAutoConfiguration

coveralls · 2017-12-07T11:13:54Z

Coverage increased (+0.02%) to 86.958% when pulling 6c3e7c7 on acsukesh:master into b162b31 on ServiceComb:master.

…nter_test code updated

coveralls · 2017-12-08T02:10:59Z

Coverage increased (+0.007%) to 86.945% when pulling 9ebe6ab on acsukesh:master into b162b31 on ServiceComb:master.

WillemJiang · 2017-12-08T10:42:18Z

...er-registry/src/main/java/io/servicecomb/springboot/starter/registry/RegistryIntializer.java

+
+  public static void initRegistry() {
+    String address = DynamicPropertyFactory.getInstance().getStringProperty("cse.rest.address", null).get();
+    if (null != address) {


If the address is null, we should write some log to notify the user, otherwise it's hard to find out the issue.

WillemJiang

Please take a look at the RegistryIntializer.initRegistry comment.

…nter_Null logs added

coveralls · 2017-12-11T03:27:29Z

Coverage decreased (-0.07%) to 86.864% when pulling 53ffe99 on acsukesh:master into b162b31 on ServiceComb:master.

WillemJiang · 2017-12-11T06:11:24Z

...er-registry/src/main/java/io/servicecomb/springboot/starter/registry/RegistryIntializer.java

        LOG.error("init registry error.", e);
      }
+    } else {
+      LOG.info("rest address is null.Service is not registered to service center");


It's better to throw the exception out instead of writing some log messages.

[JAV-435] starter to register spring cloud applications to service ce…

6ccb2ff

…nter

WillemJiang reviewed Dec 6, 2017

View reviewed changes

wujimin reviewed Dec 7, 2017

View reviewed changes

[JAV-435] starter to register spring cloud applications to service ce…

e1b51fb

…nter

liubao68 reviewed Dec 7, 2017

View reviewed changes

[JAV-435] starter to register spring cloud applications to service ce…

6c3e7c7

…nter

liubao68 reviewed Dec 7, 2017

View reviewed changes

[JAV-435] starter to register spring cloud applications to service ce…

9ebe6ab

…nter_test code updated

liubao68 approved these changes Dec 8, 2017

View reviewed changes

WillemJiang reviewed Dec 8, 2017

View reviewed changes

WillemJiang requested changes Dec 8, 2017

View reviewed changes

[JAV-435] starter to register spring cloud applications to service ce…

53ffe99

…nter_Null logs added

WillemJiang reviewed Dec 11, 2017

View reviewed changes

WillemJiang approved these changes Dec 11, 2017

View reviewed changes

WillemJiang merged commit 415124c into apache:master Dec 11, 2017

		@@ -0,0 +1,57 @@
		/*
		* Copyright 2017 Huawei Technologies Co., Ltd

[JAV-435] starter to register spring cloud applications to service center #361

[JAV-435] starter to register spring cloud applications to service center #361

Uh oh!

Conversation

acsukesh commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liubao68 Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liubao68 Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 7, 2017

Uh oh!

coveralls commented Dec 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillemJiang left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acsukesh commented Dec 6, 2017 •

edited

Loading

liubao68 Dec 7, 2017 •

edited

Loading

liubao68 Dec 7, 2017 •

edited

Loading