kfaraz commented on code in PR #16266:
URL: https://github.com/apache/druid/pull/16266#discussion_r1564837520
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java:
##########
@@ -379,6 +379,11 @@ public TaskStatus call()
command.add("true");
}
+ if (task.doesntNeedLookups()) {
+ command.add("--doesntNeedLookups");
Review Comment:
The parameter should be called `--loadLookups` instead, and should take
values `all`, `none`, `onDemand`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -331,4 +331,9 @@ static TaskInfo<TaskIdentifier, TaskStatus>
toTaskIdentifierInfo(TaskInfo<Task,
taskInfo.getTask().getMetadata()
);
}
+
+ default boolean doesntNeedLookups()
Review Comment:
The method should rather be called `needsLookups()` or `loadLookups()`.
There is no point naming this method in the negative.
Also, since we want to extend this feature in the future to be able to load
lookups on demand, I think we should return an enum here with possible values
`NONE`, `ALL`, `ON_DEMAND`.
For the time-being, the behaviour for `ALL` and `ON_DEMAND` would be the
same. But later, `ON_DEMAND` can be optimized to load only required lookups.
So, overall:
```java
default LookupLoadingMode needsLookups()
{
return LookupLoadingMode.ALL;
}
```
--
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]
