Jackie-Jiang commented on code in PR #15284:
URL: https://github.com/apache/pinot/pull/15284#discussion_r2019080775
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -699,6 +723,28 @@ private RebalanceSummaryResult
calculateDryRunSummary(Map<String, Map<String, St
serverSegmentChangeInfoMap.put(server, new
RebalanceSummaryResult.ServerSegmentChangeInfo(serverStatus,
totalNewSegments, totalExistingSegments, segmentsAdded,
segmentsDeleted, segmentsUnchanged,
instanceToTagsMap.getOrDefault(server, null)));
+ List<String> serverTags = getServerTag(server);
+ Set<String> relevantTags = new HashSet<>(serverTags);
+ relevantTags.retainAll(tagsInfoMap.keySet());
+ // The segments remain unchanged or need to download will be accounted
to every tag associated with this
+ // server instance
+ if (relevantTags.isEmpty()) {
Review Comment:
Is this possible? When we reach this, it indicates a bug in the code because
segments should never go to these servers
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -662,6 +663,29 @@ private RebalanceSummaryResult
calculateDryRunSummary(Map<String, Map<String, St
Set<String> serversRemoved = new HashSet<>();
Set<String> serversUnchanged = new HashSet<>();
Set<String> serversGettingNewSegments = new HashSet<>();
+ Map<String, RebalanceSummaryResult.TagInfo> tagsInfoMap = new HashMap<>();
+ String serverTenantName = tableConfig.getTenantConfig().getServer();
+ if (serverTenantName != null) {
+ String serverTenantTag =
+ TagNameUtils.getServerTagForTenant(serverTenantName,
tableConfig.getTableType());
+ tagsInfoMap.put(serverTenantTag,
+ new RebalanceSummaryResult.TagInfo(serverTenantTag));
+ }
+ if (tableConfig.getInstanceAssignmentConfigMap() != null) {
+ // for simplicity, including all segment types present in
instanceAssignmentConfigMap
+
tableConfig.getInstanceAssignmentConfigMap().values().forEach(instanceAssignmentConfig
-> {
+ if (instanceAssignmentConfig.getTagPoolConfig().isPoolBased()) {
Review Comment:
It doesn't need to be pool based. Just pick the tag without the pool based
check
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/RebalanceSummaryResult.java:
##########
@@ -161,6 +170,63 @@ public int getExpectedValueAfterRebalance() {
}
}
+ public static class TagInfo {
+ public static final String TAGS_NOT_TAGGED_WITH_TABLE = "OUTDATED_SERVERS";
Review Comment:
Consider renaming it to match the constant, e.g. `TAG_FOR_OUTDATED_SERVERS`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -662,6 +663,29 @@ private RebalanceSummaryResult
calculateDryRunSummary(Map<String, Map<String, St
Set<String> serversRemoved = new HashSet<>();
Set<String> serversUnchanged = new HashSet<>();
Set<String> serversGettingNewSegments = new HashSet<>();
+ Map<String, RebalanceSummaryResult.TagInfo> tagsInfoMap = new HashMap<>();
+ String serverTenantName = tableConfig.getTenantConfig().getServer();
Review Comment:
Within `TenantConfig`, there is also `TagOverrideConfig` which you also want
to include
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -699,6 +723,28 @@ private RebalanceSummaryResult
calculateDryRunSummary(Map<String, Map<String, St
serverSegmentChangeInfoMap.put(server, new
RebalanceSummaryResult.ServerSegmentChangeInfo(serverStatus,
totalNewSegments, totalExistingSegments, segmentsAdded,
segmentsDeleted, segmentsUnchanged,
instanceToTagsMap.getOrDefault(server, null)));
+ List<String> serverTags = getServerTag(server);
+ Set<String> relevantTags = new HashSet<>(serverTags);
+ relevantTags.retainAll(tagsInfoMap.keySet());
+ // The segments remain unchanged or need to download will be accounted
to every tag associated with this
+ // server instance
+ if (relevantTags.isEmpty()) {
+ LOGGER.warn("Server: {} was assigned to table: {} but does not have
any relevant tags", server,
+ tableNameWithType);
+
+ RebalanceSummaryResult.TagInfo tagsInfo =
+
tagsInfoMap.computeIfAbsent(RebalanceSummaryResult.TagInfo.TAGS_NOT_TAGGED_WITH_TABLE,
+ RebalanceSummaryResult.TagInfo::new);
+ tagsInfo.increaseNumSegmentsUnchanged(segmentsUnchanged);
+ tagsInfo.increaseNumSegmentsToDownload(segmentsAdded);
+ tagsInfo.increaseNumServerParticipants(1);
+ }
+ for (String tag : relevantTags) {
Review Comment:
(minor) This can be put in the `else`
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
