kfaraz commented on code in PR #17863:
URL: https://github.com/apache/druid/pull/17863#discussion_r2032564075
##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCluster.java:
##########
@@ -58,8 +61,17 @@ private DruidCluster(
historicals,
holders -> CollectionUtils.newTreeSet(Comparator.naturalOrder(),
holders)
);
+ this.managedHistoricals = CollectionUtils.mapValues(
+ historicals,
+ holders -> CollectionUtils.newTreeSet(Comparator.naturalOrder(),
+ holders.stream()
+ .filter(serverHolder ->
!serverHolder.isUnmanaged())
+ .collect(Collectors.toList())
Review Comment:
Nit: Maybe do this operation and assign to a variable before calling
`CollectionUtils.mapValues()` for better readability.
##########
server/src/main/java/org/apache/druid/server/coordinator/ServerHolder.java:
##########
@@ -238,6 +264,7 @@ public long getAvailableSize()
public boolean canLoadSegment(DataSegment segment)
{
return !isDecommissioning
+ && !isUnmanaged
Review Comment:
I think this condition should not be needed here anymore.
It is also a little misleading since the server may be able to load the
segment in some other context (say in `CloneHistoricals` duty).
##########
server/src/main/java/org/apache/druid/server/coordinator/stats/Stats.java:
##########
@@ -98,6 +104,8 @@ public static class Tier
= CoordinatorStat.toDebugAndEmit("maxRepFactor",
"tier/replication/factor");
public static final CoordinatorStat HISTORICAL_COUNT
= CoordinatorStat.toDebugAndEmit("numHistorical",
"tier/historical/count");
+ public static final CoordinatorStat CLONE_COUNT
+ = CoordinatorStat.toDebugAndEmit("numHistorical",
"tier/historical/clone/count");
Review Comment:
short name should be changed to `numClones`.
##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCluster.java:
##########
@@ -92,6 +109,11 @@ public List<ServerHolder> getAllServers()
return allServers;
}
+ public List<ServerHolder> getAllManagedServers()
Review Comment:
I think we can completely remove `getAllServers()` now since the only usage
is in `PrepareBalancerAndLoadQueues` to cancel loads if the server is
decommissioning.
This usage should also be replaced with `getAllManagedServers()`.
So that a clone is just a clone and nothing else.
Even if someone accidentally marks a server both as a clone and as a
decommissioning server, we will still treat it as a clone and not perform any
of the decomm logic.
##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCluster.java:
##########
@@ -72,6 +84,11 @@ public Map<String, NavigableSet<ServerHolder>>
getHistoricals()
return historicals;
}
+ public Map<String, NavigableSet<ServerHolder>> getManagedHistoricals()
Review Comment:
Also, the method `getHistoricalsByTier()` should be renamed to
`getManagedHistoricalsByTier` and it should use the `managedHistoricals` field
rather than `historicals`.
##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCluster.java:
##########
@@ -58,8 +61,17 @@ private DruidCluster(
historicals,
holders -> CollectionUtils.newTreeSet(Comparator.naturalOrder(),
holders)
);
+ this.managedHistoricals = CollectionUtils.mapValues(
+ historicals,
+ holders -> CollectionUtils.newTreeSet(Comparator.naturalOrder(),
+ holders.stream()
+ .filter(serverHolder ->
!serverHolder.isUnmanaged())
+ .collect(Collectors.toList())
+ )
+ );
this.brokers = Collections.unmodifiableSet(brokers);
this.allServers = initAllServers();
+ this.allManagedServers = initAllManagedServers();
Review Comment:
```suggestion
this.allManagedServers = this.allServers.stream().filter(server ->
!server.isUnmanaged()).collect(Collectors.toList());
```
##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCluster.java:
##########
@@ -72,6 +84,11 @@ public Map<String, NavigableSet<ServerHolder>>
getHistoricals()
return historicals;
}
+ public Map<String, NavigableSet<ServerHolder>> getManagedHistoricals()
Review Comment:
Add a one-line javadoc to distinguish between `getHistoricals()` and
`getManagedHistoricals()`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
