[SCB-986] Cache instances result in CseDiscoveryClient #990
Conversation
|
|
||
| @Override | ||
| public int getOrder() { | ||
| return Integer.MAX_VALUE; |
There was a problem hiding this comment.
should give a change to filter List
so set order to be max int is not a good idea
we can set it to be: (int) Short.MAX_VALUE;
|
|
||
| @Override | ||
| public DiscoveryTreeNode discovery(DiscoveryContext context, DiscoveryTreeNode parent) { | ||
| return createDiscoveryTreeNode(context, parent); |
There was a problem hiding this comment.
there is no cache logic, because create new node each time,
take a look at: org.apache.servicecomb.loadbalance.filter.ServerDiscoveryFilter
| @@ -75,5 +75,9 @@ | |||
| <groupId>org.apache.servicecomb</groupId> | |||
| <artifactId>service-registry</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
why need this new dependency?
There was a problem hiding this comment.
The DiscoveryFilter interface is defined in this package
There was a problem hiding this comment.
no , you add new core dependence
There was a problem hiding this comment.
That's for Const.RESTFUL
| List<ServiceInstance> instances = new ArrayList<>(); | ||
| for (MicroserviceInstance instance : ((Map<String, MicroserviceInstance>) parent.data()).values()) { | ||
| for (String endpoint : instance.getEndpoints()) { | ||
| URIEndpointObject uri = new URIEndpointObject(endpoint); |
There was a problem hiding this comment.
zuul can not support highway
so we need to log and ignore highway endpoints
| Map<String, MicroserviceInstance> servers = serversVersionedCache.data(); | ||
| List<ServiceInstance> instances = new ArrayList<>(servers.size()); | ||
| List<ServiceInstance> instances = null; | ||
| instances = serversVersionedCache.data(); |
There was a problem hiding this comment.
merge line 93 and 94 to one line.
| List<ServiceInstance> instances = null; | ||
| instances = serversVersionedCache.data(); | ||
|
|
||
| //Map<String, MicroserviceInstance> servers = serversVersionedCache.data(); |
There was a problem hiding this comment.
remote line from 96 to 104
|
@wujimin All are fixed, could you please check again? |
| protected DiscoveryTreeNode createDiscoveryTreeNode(DiscoveryContext context, | ||
| DiscoveryTreeNode parent) { | ||
| String serviceName = context.getInputParameters(); | ||
| List<ServiceInstance> instances = new ArrayList<>(); |
| @@ -44,24 +51,55 @@ public String description() { | |||
|
|
|||
| @Override | |||
| public List<ServiceInstance> getInstances(final String serviceId) { | |||
| class InstanceDiscoveryFilter implements DiscoveryFilter { | |||
There was a problem hiding this comment.
put class InstanceDiscoveryFilter in "getInstances", make "getInstances" logic not clear
There was a problem hiding this comment.
Will move to a separate class
| } | ||
| return instances; | ||
|
|
||
| return serversVersionedCache.data(); |
There was a problem hiding this comment.
what happened when serversVersionedCache is empty?
There was a problem hiding this comment.
It's OK for the versionedCache to be empty. As it will not be NULL. When there are associated filters and the children data is null, an Exception will be raised.
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import org.apache.servicecomb.core.Const; |
There was a problem hiding this comment.
remove java-chassis-core dependency
define a new const "rest" or just hard code is enough.
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles, where you replaceSCB-XXXwith the appropriate JIRA issue.mvn clean installto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.