jihoonson commented on a change in pull request #9971:
URL: https://github.com/apache/druid/pull/9971#discussion_r436307096
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java
##########
@@ -327,6 +327,13 @@ public TaskStatus call()
command.add(nodeType);
}
+ // If the task type is queryable, we need to load
broadcast segments on the peon, used for
+ // join queries
+ if (task.supportsQueries()) {
+ command.add("--loadBroadcastSegments");
Review comment:
Hmm, the CI failure doesn't like missing tests for this change. Thinking
about it for a bit, what will happen if the peon always loads the stuffs for
loading broadcast segments? If I'm reading the code correctly, the coordinator
will not broadcast segments to batch tasks no matter whether they are running
on neither middleManagers nor indexers. [The realtime tasks announce themselves
when they run on middleManagers or don't announce when they run on
indexers](https://github.com/apache/druid/blob/master/indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunner.java#L420-L423),
but the batch tasks never announce themselves. So, it seems like, if the peon
always loaded the stuffs for broadcast segment loading, those stuffs could be
loaded for batch tasks as well but wouldn't do anything? If that's the case,
I'm fine with always loading them rather than adding this new parameter and
tests for it.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
