[GitHub] [skywalking] wu-sheng commented on a change in pull request #3619: Fix more resonable error

[GitBox <gi...@apache.org>]

wu-sheng commented on a change in pull request #3619: Fix more resonable error
URL: https://github.com/apache/skywalking/pull/3619#discussion_r334877823
 
 

 ##########
 File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/remote/client/RemoteClientManager.java
 ##########
 @@ -181,29 +186,27 @@ private void switchCurrentClients() {
      *
      * @param remoteInstances Remote instance collection by query cluster config.
      */
-    private synchronized void reBuildRemoteClients(List<RemoteInstance> remoteInstances) {
-        getFreeClients().clear();
+    private void reBuildRemoteClients(List<RemoteInstance> remoteInstances) {
+        final Map<Address, RemoteClientAction> closeRemoteClient = this.usingClients.stream()
+                .collect(Collectors.toMap(RemoteClient::getAddress, client -> new RemoteClientAction(client, Action.Close)));
 
-        Map<Address, RemoteClient> remoteClients = new HashMap<>();
-        getRemoteClient().forEach(client -> remoteClients.put(client.getAddress(), client));
+        final Map<Address, RemoteClientAction> createRemoteClient = remoteInstances.stream()
+                .collect(Collectors.toMap(RemoteInstance::getAddress, remote -> new RemoteClientAction(null, Action.Create)));
 
-        Map<Address, Action> tempRemoteClients = new HashMap<>();
-        getRemoteClient().forEach(client -> tempRemoteClients.put(client.getAddress(), Action.Close));
+        final Set<Address> unChangeAddresses = Sets.intersection(closeRemoteClient.keySet(), createRemoteClient.entrySet());
 
-        remoteInstances.forEach(remoteInstance -> {
-            if (tempRemoteClients.containsKey(remoteInstance.getAddress())) {
-                tempRemoteClients.put(remoteInstance.getAddress(), Action.Leave);
-            } else {
-                tempRemoteClients.put(remoteInstance.getAddress(), Action.Create);
-            }
-        });
+        unChangeAddresses.stream()
+                .filter(closeRemoteClient::containsKey)
+                .forEach(unChangeAddress -> closeRemoteClient.get(unChangeAddress).setAction(Action.Unchanged));
 
-        tempRemoteClients.forEach((address, action) -> {
-            switch (action) {
-                case Leave:
-                    if (remoteClients.containsKey(address)) {
-                        getFreeClients().add(remoteClients.get(address));
-                    }
+        unChangeAddresses.forEach(createRemoteClient::remove);
+        closeRemoteClient.putAll(createRemoteClient);
+
+        getFreeClients().clear(); //clean free client list, for build a new list
 
 Review comment:
   If you still use clear, it is not safe. Still could get a chance to have list ref, than cleared by here. Right?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services