[GitHub] [druid] jon-wei commented on a change in pull request #11056: Add paramter to loadstatus API to compute underdeplication against cluster view

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

jon-wei commented on a change in pull request #11056:
URL: https://github.com/apache/druid/pull/11056#discussion_r605322228



##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -584,6 +580,59 @@ public void runCompactSegmentsDuty()
     compactSegmentsDuty.run();
   }
 
+  private Map<String, Object2LongMap<String>> computeUnderReplicationCountsPerDataSourcePerTierForSegmentsInternal(
+      Iterable<DataSegment> dataSegments,
+      boolean computeUsingClusterView
+  )
+  {
+    final Map<String, Object2LongMap<String>> underReplicationCountsPerDataSourcePerTier = new HashMap<>();
+
+    if (segmentReplicantLookup == null) {
+      throw new ServiceUnavailableException(
+          "Coordinator segment replicant lookup is not initialized yet. Try again later.");
+    }
+
+    if (computeUsingClusterView && cluster == null) {
+      throw new ServiceUnavailableException(
+          "coordinator hasn't populated information about cluster yet, try again later");
+    }
+
+    final DateTime now = DateTimes.nowUtc();
+
+    for (final DataSegment segment : dataSegments) {
+      final List<Rule> rules = metadataRuleManager.getRulesWithDefault(segment.getDataSource());
+
+      for (final Rule rule : rules) {
+        if (!rule.appliesTo(segment, now)) {
+          // Rule did not match. Continue to the next Rule.
+          continue;
+        }
+        if (!rule.canLoadSegments()) {
+          // Rule matched but rule does not and cannot load segments.
+          // Hence, there is no need to update underReplicationCountsPerDataSourcePerTier map
+          break;
+        }
+
+        if (computeUsingClusterView && (rule instanceof LoadRule)) {

Review comment:
       The `(rule instanceof LoadRule)` should be removed, since `BroadcastDistributionRule` can also load segments (it also returns true for `canLoadSegments()`, which is already checked before this)

##########
File path: server/src/main/java/org/apache/druid/server/coordinator/rules/LoadRule.java
##########
@@ -119,6 +119,34 @@ public void updateUnderReplicated(
     });
   }
 
+  @Override
+  public void updateUnderReplicatedWithClusterView(

Review comment:
       Can you add an implementation for `BroadcastDistributionRule` as well? I think the implementation there could just call `updateUnderReplicated` and ignore the `cluster` parameter, since it's already looking at the available servers in the cluster.

##########
File path: server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java
##########
@@ -1201,7 +1201,7 @@ public void testMarkSegmentsAsUnusedInvalidPayloadBothArguments()
   public void testGetDatasourceLoadstatusForceMetadataRefreshNull()
   {
     DataSourcesResource dataSourcesResource = new DataSourcesResource(inventoryView, segmentsMetadataManager, null, null, null, null);
-    Response response = dataSourcesResource.getDatasourceLoadstatus("datasource1", null, null, null, null);
+    Response response = dataSourcesResource.getDatasourceLoadstatus("datasource1", null, null, null, null, null);

Review comment:
       Can you add tests that use the new parameter?




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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org