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

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

dmsolr commented on a change in pull request #3619: Fix more resonable error
URL: https://github.com/apache/skywalking/pull/3619#discussion_r334956300
 
 

 ##########
 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:
   The key point is, clear and copyOf are not atomic operations. You need to understand that.

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