You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/10/25 08:46:31 UTC

[PR] [minion] Allow generators to validate effective minion tags [pinot]

xiangfu0 opened a new pull request, #20036:
URL: https://github.com/apache/pinot/pull/20036

## Description

A scheduling request can override a table's minion tag, so a generator that only checks table configuration cannot validate the pool that will receive its tasks.

Add a default no-op `PinotTaskGenerator.validateMinionInstanceTag(TableConfig, String)` hook and call it with the resolved tag before grouping or generating task configs. A rejected table uses the existing per-table error handling; other generators retain their current behavior. This provides the shared scheduler boundary needed by the DELTA consumers in startreedata/startree-pinot#4903 and #4904; consumer validation is separate from this API change.

## Validation

- On unchanged `c5ac29be0e6a3d5c5fae5c0ed79ece50f87847d9`, two wrong-tag cases reproduced generator side effects; matching/default-tag and ordinary override controls passed. With the hook, all **7 focused stateless-scheduler cases pass**, zero failures/errors/skips, including two existing scheduling controls. Rejected tags produce no generation or submission.
- JDK25, source-built reactor dependencies, no Pinot project Maven artifacts on the actual test classpath. The exact frozen three-file source was committed as `c2f8e5b31c4f940e156e631289ebcf0a7d909e3c`.
- Four scoped controller quality goals passed. Scoped compilation retained three existing metric deprecation warnings and added none on changed lines.
- Independent local review covered all three changed files and the source/runtime binding. Current-head hosted CI is pending.

## Release Notes

Add an optional task-generator hook for validating the effective minion tag before task generation.

--
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] [minion] Allow generators to validate effective minion tags [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.

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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] [minion] Allow generators to validate effective minion tags [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.

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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] [minion] Allow generators to validate effective minion tags [pinot]

Posted by "xiangfu0 (via GitHub) via commits" <co...@pinot.apache.org>.

xiangfu0 closed pull request #20036: [minion] Allow generators to validate effective minion tags
URL: https://github.com/apache/pinot/pull/20036


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] [minion] Allow generators to validate effective minion tags [pinot]

Posted by "xiangfu0 (via GitHub) via commits" <co...@pinot.apache.org>.

xiangfu0 commented on code in PR #20036:
URL: https://github.com/apache/pinot/pull/20036#discussion_r4430647429


##########
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

Review Comment:
   A rejection here does more than skip the table. If every table of a task type is rejected, `scheduleTask` throws, and `doScheduleTasks` rethrows that, so a multi-type `POST /tasks/schedule?tableName=T&minionInstanceTag=X` (no `taskType`) fails as a whole. In a local repro with a lax type and a strict type on one table, the lax task was submitted first. The strict rejection then turned the request into a `RuntimeException`, and the lax task name was dropped from the response. With the opposite HashMap order, the lax type would never be scheduled. The mechanism already existed, but this hook makes it reachable through a routine override. Consider documenting this in `@throws`, or recording a rejection as a per-table skip rather than a type-level failure.
   
   AI-generated review via IntentLab · Model: Claude Opus 5.5 (claude-opus-5-5)



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] [minion] Allow generators to validate effective minion tags [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.

codecov-commenter commented on PR #20036:
URL: https://github.com/apache/pinot/pull/20036#issuecomment-6396013964

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/20036?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 59.09%. Comparing base ([`c5ac29b`](https://app.codecov.io/gh/apache/pinot/commit/c5ac29be0e6a3d5c5fae5c0ed79ece50f87847d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c2f8e5b`](https://app.codecov.io/gh/apache/pinot/commit/c2f8e5b31c4f940e156e631289ebcf0a7d909e3c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   > :exclamation:  There is a different number of reports uploaded between BASE (c5ac29b) and HEAD (c2f8e5b). Click for more details.
   > 
   > <details><summary>HEAD has 24 uploads less than BASE</summary>
   >
   >| Flag | BASE (c5ac29b) | HEAD (c2f8e5b) |
   >|------|------|------|
   >|lane-a|2|0|
   >|integration|4|0|
   >|java-25|6|1|
   >|integration1|2|0|
   >|temurin|6|1|
   >|lane-b|2|0|
   >|integration2|2|0|
   >|unittests|2|1|
   >|unittests2|1|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #20036       +/-   ##
   =============================================
   - Coverage     69.92%   59.09%   -10.83%     
   + Complexity     1519        1     -1518     
   =============================================
     Files          3557     2742      -815     
     Lines        236276   173303    -62973     
     Branches      37939    28602     -9337     
   =============================================
   - Hits         165210   102413    -62797     
   - Misses        58353    62241     +3888     
   + Partials      12713     8649     -4064     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration1](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration2](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-25](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <ø> (-10.83%)` | :arrow_down: |
   | [lane-a](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [lane-b](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <ø> (-10.83%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <ø> (-10.83%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <ø> (-0.01%)` | :arrow_down: |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/20036/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/20036?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org