xiangfu0 commented on code in PR #20036:
URL: https://github.com/apache/pinot/pull/20036#discussion_r4370776561
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -945,6 +945,7 @@ protected TaskSchedulingInfo
scheduleTask(PinotTaskGenerator taskGenerator, List
}
String minionInstanceTag = minionInstanceTagForTask != null ?
minionInstanceTagForTask
: taskGenerator.getMinionInstanceTag(tableConfig);
+ taskGenerator.validateMinionInstanceTag(tableConfig,
minionInstanceTag);
Review Comment:
This covers the periodic/scheduleTasks path nicely. Heads up though: the
ad-hoc `createTask` path (around line 284) also honors a `minionInstanceTag`
override from the request but never calls this hook, so a generator enforcing a
pool there would be bypassed. Worth validating in both places, or noting why
ad-hoc is out of scope.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/PinotTaskGenerator.java:
##########
@@ -97,6 +98,16 @@ default String getMinionInstanceTag(TableConfig tableConfig)
{
return CommonConstants.Helix.UNTAGGED_MINION_INSTANCE;
}
+ /// Validates the effective minion instance tag before scheduled task
generation.
+ /// The effective tag includes any scheduling request override. The default
implementation permits every tag.
+ /// Implementations must only validate: scheduling paths may invoke this
hook more than once.
+ ///
+ /// @param tableConfig table whose tasks are being scheduled
+ /// @param minionInstanceTag effective tag, which may be null
+ /// @throws IllegalArgumentException if the tag is incompatible with the
table's tasks
+ default void validateMinionInstanceTag(TableConfig tableConfig, @Nullable
String minionInstanceTag) {
Review Comment:
Good call documenting that this may be invoked more than once and must stay
side-effect-free. Since the tag can be null, maybe add a one-line note on what
a null tag means for implementers (effective untagged vs. unresolved) so
overrides handle it consistently.
--
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]
