xiangfu0 commented on code in PR #20036:
URL: https://github.com/apache/pinot/pull/20036#discussion_r4371632173
##########
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:
Nit/scope question: the ad-hoc `createTask(...)` path also resolves an
effective tag from the `minionInstanceTag` override (around line 285) but
doesn't call this hook. Since the PR's motivation is request-level tag
overrides, is leaving ad-hoc unvalidated intentional? Fine to defer, but worth
a note in the description.
##########
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:
Minor: a rejection here is caught as a task-*generation* error, so it bumps
the generation error gauge / error-run history. The existing invalid-tag check
(submit loop) is classified as a *scheduling* error instead. Not blocking, just
a slight observability inconsistency to be aware of.
--
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]
