[GitHub] [accumulo] keith-turner commented on a diff in pull request #3479: TabletGroupWatcher update to handle servers being shutdown

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #3479:
URL: https://github.com/apache/accumulo/pull/3479#discussion_r1224783610


##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -149,16 +149,13 @@ private static class TabletLists {
     private final List<TabletLocationState> suspendedToGoneServers = new ArrayList<>();
     private final Map<KeyExtent,UnassignedTablet> unassigned = new HashMap<>();
     private final Map<TServerInstance,List<Path>> logsForDeadServers = new TreeMap<>();
-    // read only lists of tablet servers
-    private final SortedMap<TServerInstance,TabletServerStatus> currentTServers;
-    private final SortedMap<TServerInstance,TabletServerStatus> destinations;
+    // read only list of tablet servers that are not shutting down
+    private final SortedMap<TServerInstance,TabletServerStatus> validAssignmentDestinations;
 
     public TabletLists(Manager m, SortedMap<TServerInstance,TabletServerStatus> curTServers) {
       var destinationsMod = new TreeMap<>(curTServers);
-      // Don't move tablets to servers that are shutting down
       destinationsMod.keySet().removeAll(m.serversToShutdown);

Review Comment:
   This is a pre-existing problem, not something introduced in this PR.  It could be fixed in this PR, but does not have to be.  I noticed that serversToShutdown is a syncronized set and the removeAll method may iterator over it.  The following is how the Collections javadoc recommend iterating over synchronized sets.
   
   ```suggestion
         synchronized(m.serversToShutdown){
           destinationsMod.keySet().removeAll(m.serversToShutdown);
         }
   ```



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

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org