ctubbsii commented on code in PR #3479:
URL: https://github.com/apache/accumulo/pull/3479#discussion_r1224868119
##########
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 isn't necessary. Only calls to `.iterator()`, `.spliterator()`,
`.stream()`, and `.parallelStream()` methods need to be manually synchronized,
because these operations are not closed over the operation. `.removeAll()` is
explicitly synchronized on the internal mutex in the synchronized set, while it
waits for `.removeAll()` to complete on the wrapped `HashSet`. Additionally,
this wouldn't even work, because the collection is not synchronized on `this`,
but on a private `this.mutex` instead inside `SynchronizedCollection`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
