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: [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]
