Re: [PR] Consolidate RetrieveSegmentsToReplaceAction into RetrieveUsedSegmentsAction (druid)

["kfaraz (via GitHub)" <gi...@apache.org>]

kfaraz commented on code in PR #15699:
URL: https://github.com/apache/druid/pull/15699#discussion_r1460169688


##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUsedSegmentsAction.java:
##########
@@ -49,6 +61,8 @@
  */
 public class RetrieveUsedSegmentsAction implements TaskAction<Collection<DataSegment>>
 {
+  private static final Logger log = new Logger(RetrieveSegmentsToReplaceAction.class);

Review Comment:
   ```suggestion
     private static final Logger log = new Logger(RetrieveUsedSegmentsAction.class);
   ```



##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUsedSegmentsAction.java:
##########
@@ -113,11 +139,84 @@ public TypeReference<Collection<DataSegment>> getReturnTypeReference()
 
   @Override
   public Collection<DataSegment> perform(Task task, TaskActionToolbox toolbox)
+  {
+    if (!replace) {

Review Comment:
   The new method should also have a javadoc saying that it returns a consistent view of segments and why it is needed.



##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUsedSegmentsAction.java:
##########
@@ -113,11 +139,84 @@ public TypeReference<Collection<DataSegment>> getReturnTypeReference()
 
   @Override
   public Collection<DataSegment> perform(Task task, TaskActionToolbox toolbox)
+  {
+    if (!replace) {

Review Comment:
   Cleaner to do:
   
   ```
   if (isReplaceTask()) {
     return newMethodWhichDoesTheRightThingForReplaceTask();
   } else {
     return retrieveUsedSegments(toolbox);
   }
   
   private boolean isReplaceTask() {
      return replace && task.getDatasource().equals(dataSource);
   }
   ```



##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUsedSegmentsAction.java:
##########
@@ -85,6 +103,8 @@ public RetrieveUsedSegmentsAction(
 
     // Defaulting to the former behaviour when visibility wasn't explicitly specified for backward compatibility
     this.visibility = visibility != null ? visibility : Segments.ONLY_VISIBLE;
+
+    this.replace = replace != null ? replace : false;

Review Comment:
   Can this not be determined automatically while running the action? Only tasks with a `REPLACE` lock would have this as true, right?



-- 
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-unsubscribe@druid.apache.org

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