[GitHub] [servicecomb-pack] fengbaicanhe commented on a change in pull request #454: fix when server.port=0, health check fail and also fix lose consul metadata, such as 'secure=false'

2019-04-20 Thread GitBox 
fengbaicanhe commented on a change in pull request #454: fix when 
server.port=0, health check fail and also fix lose consul metadata, such as 
'secure=false'
URL: https://github.com/apache/servicecomb-pack/pull/454#discussion_r277151320
 
 

 ##
 File path: 
alpha/alpha-spring-cloud-starter-consul/src/main/java/org/apache/servicecomb/pack/alpha/server/discovery/consul/AlphaConsulAutoConfiguration.java
 ##
 @@ -114,7 +114,11 @@ public void 
listenInstanceRegisteredEvent(InstanceRegisteredEvent instanceRegist
 NewService newservice =  new NewService();
 newservice.setName(service.getServiceName());
 newservice.setId(service.getServiceId());
-List tags = consulDiscoveryProperties.getTags();
+newservice.setAddress(service.getAddress());
 
 Review comment:
   ~~I think so, the  IP address is no need to change~~ [see 
blow](https://github.com/apache/servicecomb-pack/pull/454#discussion_r277152930)


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [servicecomb-pack] fengbaicanhe commented on a change in pull request #454: fix when server.port=0, health check fail and also fix lose consul metadata, such as 'secure=false'

2019-04-20 Thread GitBox 
fengbaicanhe commented on a change in pull request #454: fix when 
server.port=0, health check fail and also fix lose consul metadata, such as 
'secure=false'
URL: https://github.com/apache/servicecomb-pack/pull/454#discussion_r277151320
 
 

 ##
 File path: 
alpha/alpha-spring-cloud-starter-consul/src/main/java/org/apache/servicecomb/pack/alpha/server/discovery/consul/AlphaConsulAutoConfiguration.java
 ##
 @@ -114,7 +114,11 @@ public void 
listenInstanceRegisteredEvent(InstanceRegisteredEvent instanceRegist
 NewService newservice =  new NewService();
 newservice.setName(service.getServiceName());
 newservice.setId(service.getServiceId());
-List tags = consulDiscoveryProperties.getTags();
+newservice.setAddress(service.getAddress());
 
 Review comment:
   ~~I think so, the  IP address is no need to change~~


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [servicecomb-pack] fengbaicanhe commented on a change in pull request #454: fix when server.port=0, health check fail and also fix lose consul metadata, such as 'secure=false'

2019-04-20 Thread GitBox 
fengbaicanhe commented on a change in pull request #454: fix when 
server.port=0, health check fail and also fix lose consul metadata, such as 
'secure=false'
URL: https://github.com/apache/servicecomb-pack/pull/454#discussion_r277152930
 
 

 ##
 File path: 
alpha/alpha-spring-cloud-starter-consul/src/main/java/org/apache/servicecomb/pack/alpha/server/discovery/consul/AlphaConsulAutoConfiguration.java
 ##
 @@ -114,7 +114,11 @@ public void 
listenInstanceRegisteredEvent(InstanceRegisteredEvent instanceRegist
 NewService newservice =  new NewService();
 newservice.setName(service.getServiceName());
 newservice.setId(service.getServiceId());
-List tags = consulDiscoveryProperties.getTags();
+newservice.setAddress(service.getAddress());
+newservice.setPort(service.getServicePort());
 
 Review comment:
   [As i describe my development environment 
above](https://github.com/apache/servicecomb-pack/pull/454#discussion_r277151707),
 the port will be updated to '0' after call method 
   ```java
   @EventListener
   public void listenInstanceRegisteredEvent(InstanceRegisteredEvent 
instanceRegisteredEvent)
   ```
   
   I know when project startup, when set server.port=0, 
   
   - spring will generate a random port like 56701
   - then spring use this port registered to consul,
   - after service registered, spring will publish a `InstanceRegisteredEvent`  
event
   
   and alpha server subscribe the event, re-register service instance with 
`alpha-server-port` and `alpha-server-host` tags while alphaServerPort is set 
to zero
   
   I think the problem is `NewService newservice =  new NewService();` **after 
this line, the default health check host and port will be set to null**
   
   and in actually register code in blow
   ```java
@Override
public Response agentServiceRegister(NewService newService, 
String token) {
UrlParameters tokenParam = token != null ? new 
SingleUrlParameters("token", token) : null;
   
String json = GsonFactory.getGson().toJson(newService);
RawResponse rawResponse = 
rawClient.makePutRequest("/v1/agent/service/register", json, tokenParam);
   
if (rawResponse.getStatusCode() == 200) {
return new Response(null, rawResponse);
} else {
throw new OperationException(rawResponse);
}
}
   ```
   it only serialize `newService` to json, and **the serialization only include 
non-null value**, so the health check host and port lost
   
   So re-think about this problem, I think the host and port should be set to 
`newService` is necessary
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [servicecomb-pack] fengbaicanhe commented on a change in pull request #454: fix when server.port=0, health check fail and also fix lose consul metadata, such as 'secure=false'

2019-04-20 Thread GitBox 
fengbaicanhe commented on a change in pull request #454: fix when 
server.port=0, health check fail and also fix lose consul metadata, such as 
'secure=false'
URL: https://github.com/apache/servicecomb-pack/pull/454#discussion_r277151707
 
 

 ##
 File path: 
alpha/alpha-spring-cloud-starter-consul/src/main/java/org/apache/servicecomb/pack/alpha/server/discovery/consul/AlphaConsulAutoConfiguration.java
 ##
 @@ -114,7 +114,11 @@ public void 
listenInstanceRegisteredEvent(InstanceRegisteredEvent instanceRegist
 NewService newservice =  new NewService();
 newservice.setName(service.getServiceName());
 newservice.setId(service.getServiceId());
-List tags = consulDiscoveryProperties.getTags();
+newservice.setAddress(service.getAddress());
 
 Review comment:
   Sorry about i do not describe my develop environment clearly, and I am sorry 
for submitting the code without doing a full test( only tested in below 
environment ).
   
   **version details:** 
   
   > spring boot version: 2.1.4.RELEASE
   > spring-cloud-starter-consul version: 2.1.1.RELEASE
   > consul version: 1.4.4
   > alpha server version: 0.4.0
   
   **application.properties**
   
   > server.port=0
   > alpha.server.host=0.0.0.0
   > alpha.server.port=0
   > alpha.server.initialPort=34500
   
   **One more should be attention**
   I **use alpha server as a dependency** in my spring boot project
   
   I use 
[spring-boot-admin-server](https://github.com/codecentric/spring-boot-admin) 
for service health monitor
   
   As mentioned above, if `newservice.setPort(service.getServicePort());` this 
line is missing, consul will register service's health check port as 0, and 
this will cause health check fails


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [servicecomb-pack] fengbaicanhe commented on a change in pull request #454: fix when server.port=0, health check fail and also fix lose consul metadata, such as 'secure=false'

2019-04-20 Thread GitBox 
fengbaicanhe commented on a change in pull request #454: fix when 
server.port=0, health check fail and also fix lose consul metadata, such as 
'secure=false'
URL: https://github.com/apache/servicecomb-pack/pull/454#discussion_r277151320
 
 

 ##
 File path: 
alpha/alpha-spring-cloud-starter-consul/src/main/java/org/apache/servicecomb/pack/alpha/server/discovery/consul/AlphaConsulAutoConfiguration.java
 ##
 @@ -114,7 +114,11 @@ public void 
listenInstanceRegisteredEvent(InstanceRegisteredEvent instanceRegist
 NewService newservice =  new NewService();
 newservice.setName(service.getServiceName());
 newservice.setId(service.getServiceId());
-List tags = consulDiscoveryProperties.getTags();
+newservice.setAddress(service.getAddress());
 
 Review comment:
   I think so, the  IP address is no need to change


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] [servicecomb-pack] fengbaicanhe commented on a change in pull request #454: fix when server.port=0, health check fail and also fix lose consul metadata, such as 'secure=false'

2019-04-20 Thread GitBox 
fengbaicanhe commented on a change in pull request #454: fix when 
server.port=0, health check fail and also fix lose consul metadata, such as 
'secure=false'
URL: https://github.com/apache/servicecomb-pack/pull/454#discussion_r277151319
 
 

 ##
 File path: 
alpha/alpha-spring-cloud-starter-consul/src/main/java/org/apache/servicecomb/pack/alpha/server/discovery/consul/AlphaConsulAutoConfiguration.java
 ##
 @@ -114,7 +114,11 @@ public void 
listenInstanceRegisteredEvent(InstanceRegisteredEvent instanceRegist
 NewService newservice =  new NewService();
 newservice.setName(service.getServiceName());
 newservice.setId(service.getServiceId());
-List tags = consulDiscoveryProperties.getTags();
+newservice.setAddress(service.getAddress());
+newservice.setPort(service.getServicePort());
+newservice.setMeta(service.getServiceMeta());
 
 Review comment:
   As you know, **Spring Cloud Consul uses Consul tags to approximate metadata 
until Consul officially supports metadata.** 
   ```
   newservice.setMeta(service.getServiceMeta());
   ```
   this line i committed is useless, it should be remove


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services