[GitHub] [druid] jihoonson commented on a change in pull request #10307: Add support for all partitioing schemes for auto compaction

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

jihoonson commented on a change in pull request #10307:
URL: https://github.com/apache/druid/pull/10307#discussion_r475882549



##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTask.java
##########
@@ -764,7 +765,28 @@ private PartitionBoundaries determineRangePartition(Collection<StringDistributio
     return Pair.of(start, stop);
   }
 
-  private static void publishSegments(TaskToolbox toolbox, Map<String, PushedSegmentsReport> reportsMap)
+  public static Function<Set<DataSegment>, Set<DataSegment>> compactionStateAnnotateFunction(

Review comment:
       Good idea! Moved this method.

##########
File path: server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskQueryTuningConfig.java
##########
@@ -158,27 +222,90 @@ public IndexSpec getIndexSpec()
     return indexSpec;
   }
 
+  @JsonProperty
+  @Nullable
+  public IndexSpec getIndexSpecForIntermediatePersists()
+  {
+    return indexSpecForIntermediatePersists;
+  }
+
   @JsonProperty
   @Nullable
   public Integer getMaxPendingPersists()
   {
     return maxPendingPersists;
   }
 
+  @JsonProperty

Review comment:
       Yes, this should be passed properly when this class is deserialized as `ParallelIndexTuningConfig`.

##########
File path: server/src/main/java/org/apache/druid/server/coordinator/duty/NewestSegmentFirstIterator.java
##########
@@ -247,17 +248,30 @@ public boolean hasNext()
     }
   }
 
-  private boolean needsCompaction(DataSourceCompactionConfig config, SegmentsToCompact candidates)
+  @VisibleForTesting
+  static PartitionsSpec findPartitinosSpecFromConfig(ClientCompactionTaskQueryTuningConfig tuningConfig)
+  {

Review comment:
       The auto compaction supported only dynamic partitioning before this PR. The deprecated `maxRowsPerSegment` and `maxTotalRows` in `DataSourceCompactionConfig` are for dynamic partitioning. Users are recommended to use `partitionsSpec` instead of them. 
   
   > Misread the code. if the maxTotalRows is null in partition spec, should it be read from tuningConfig instead of using the Long.Max?
   
   My intention is using `partitionsSpec` if it's specified and ignoring deprecated ones. I will add docs for this behaviour as well as the newly supported tuning configurations.




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