You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rohityadav1993 (via GitHub)" <gi...@apache.org> on 2025/09/09 06:50:48 UTC
[PR] configure numConcurrentTasksPerInstance for minion tenant [pinot]
rohityadav1993 opened a new pull request, #16777:
URL: https://github.com/apache/pinot/pull/16777
`Configuration` `release-notes`
## Change
Configuration to define numConcurrentTasksPerInstance at minion tenant level.
## Description
Relates to tuning performance of minion task execution in issue: https://github.com/apache/pinot/issues/16461
Pinot controller uses cluster config `numConcurrentTasksPerInstance` to control max number of tasks that can run on a minion instance.
This is not extenisble to minion tenants where as the configuratino is per taskType and not for (taskType, minion_tag).
This change allows one to define configutation as `taskType.numConcurrentTasksPerInstance` along with `numConcurrentTasksPerInstance`.
The priority order to pick up the config is: taskType.numConcurrentTasksPerInstance > numConcurrentTasksPerInstance > (DEFAULT_NUM_CONCURRENT_TASKS_PER_INSTANCE = 1)
## Test plan
- Unit tests
- local cluster test validations
--
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] configure numConcurrentTasksPerInstance for minion tenant [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #16777:
URL: https://github.com/apache/pinot/pull/16777#discussion_r2332222301
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/PinotTaskGenerator.java:
##########
@@ -68,9 +68,16 @@ default long getTaskTimeoutMs() {
}
/**
- * Returns the maximum number of concurrent tasks allowed per instance, 1 by default.
+ * Returns the maximum number of concurrent tasks allowed per instance.
Review Comment:
This should apply to the override implementation? Here we can keep the existing "1 by default" java doc.
--
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] configure numConcurrentTasksPerInstance for minion tenant [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16777:
URL: https://github.com/apache/pinot/pull/16777#issuecomment-3269333121
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16777?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 56.51%. Comparing base ([`af3dc6a`](https://app.codecov.io/gh/apache/pinot/commit/af3dc6a3b1e25b7f5e7a90dfef0ebdc9f249dd0d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b86d76c`](https://app.codecov.io/gh/apache/pinot/commit/b86d76c846dc132004a2de87a823e79133bae863?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 (af3dc6a) and HEAD (b86d76c). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (af3dc6a) | HEAD (b86d76c) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|unittests2|2|0|
>|integration|5|0|
>|integration1|2|0|
>|java-11|4|0|
>|custom-integration1|2|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16777 +/- ##
============================================
- Coverage 63.42% 56.51% -6.91%
+ Complexity 1400 700 -700
============================================
Files 3054 2394 -660
Lines 178766 133013 -45753
Branches 27399 21166 -6233
============================================
- Hits 113378 75175 -38203
+ Misses 56656 51591 -5065
+ Partials 8732 6247 -2485
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16777/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16777/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16777/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/16777/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/16777/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16777/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16777/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.51% <ø> (-6.88%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16777/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.51% <ø> (-6.91%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16777/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.51% <ø> (-6.91%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16777/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.51% <ø> (+0.03%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16777/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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/16777?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
Re: [PR] [minion] configure numConcurrentTasksPerInstance for minion tenant [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #16777:
URL: https://github.com/apache/pinot/pull/16777#discussion_r2353107941
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/TaskGeneratorUtils.java:
##########
@@ -125,4 +129,65 @@ public static String extractMinionInstanceTag(TableConfig tableConfig, String ta
}
return CommonConstants.Helix.UNTAGGED_MINION_INSTANCE;
}
+
+ /**
+ * Generic method to get configuration values from ZK cluster config with fallback hierarchy:
+ * 1. Check minion tenant specific config (if minionTag provided)
+ * 2. Check task type specific config
+ * 3. Return default value
+ *
+ * @param taskType The task type to build config keys
+ * @param configKeySuffix The suffix to append to task type for config key
+ * @param minionTag The minion tag for tenant-specific config (can be null)
+ * @param defaultValue The default value to return if no config is found
+ * @param returnType The class type for parsing the config value. Currently supported: Integer, Long, String
+ * @param clusterInfoAccessor The cluster info accessor to get config values
+ * @param <T> The return type
+ * @return The parsed configuration value or default value
+ */
+ public static <T> T getClusterMinionConfigValue(String taskType, String configKeySuffix, String minionTag,
+ T defaultValue, Class<T> returnType, ClusterInfoAccessor clusterInfoAccessor) {
+ // Priority 1: Check minion tenant specific cluster config
+ if (minionTag != null) {
+ String tenantSpecificConfigKey = taskType + "." + minionTag + configKeySuffix;
+ String configValue = clusterInfoAccessor.getClusterConfig(tenantSpecificConfigKey);
+ if (configValue != null) {
+ try {
+ return parseConfigValue(configValue, returnType);
+ } catch (Exception e) {
+ LOGGER.error("Invalid config {}: '{}'", tenantSpecificConfigKey, configValue, e);
+ }
+ }
+ }
+
+ // Priority 2: Check task type specific cluster config
+ String taskTypeConfigKey = taskType + configKeySuffix;
+ String configValue = clusterInfoAccessor.getClusterConfig(taskTypeConfigKey);
+ if (configValue != null) {
+ try {
+ return parseConfigValue(configValue, returnType);
+ } catch (Exception e) {
+ LOGGER.error("Invalid config {}: '{}'", taskTypeConfigKey, configValue, e);
+ }
+ }
+
+ // Priority 3: Default value
+ return defaultValue;
+ }
+
+ /**
+ * Helper method to parse config values to the appropriate type
+ */
+ @SuppressWarnings("unchecked")
+ private static <T> T parseConfigValue(String configValue, Class<T> returnType) {
Review Comment:
nit: this means that if one were to add a new config that takes in a different type, it would fail at Runtime, unless there's UT coverage for it.
Just to be safe, maybe you can also cover the Double and Float types here.
--
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] configure numConcurrentTasksPerInstance for minion tenant [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #16777:
URL: https://github.com/apache/pinot/pull/16777#discussion_r2349528876
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/BaseTaskGenerator.java:
##########
@@ -68,17 +68,35 @@ public long getTaskTimeoutMs() {
}
@Override
- public int getNumConcurrentTasksPerInstance() {
+ public int getNumConcurrentTasksPerInstance(String minionTag) {
String taskType = getTaskType();
- String configKey = taskType + MinionConstants.NUM_CONCURRENT_TASKS_PER_INSTANCE_KEY_SUFFIX;
- String configValue = _clusterInfoAccessor.getClusterConfig(configKey);
+
+ // Priority 1: Check minion tenant specific cluster config
+ if (minionTag != null) {
Review Comment:
I think this logic of first checking tenant level config, and then task level config, should be moved to another generic method, to ensure that the same semantics can get re-used for all minion cluster configs.
--
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] configure numConcurrentTasksPerInstance for minion tenant [pinot]
Posted by "rohityadav1993 (via GitHub)" <gi...@apache.org>.
rohityadav1993 commented on code in PR #16777:
URL: https://github.com/apache/pinot/pull/16777#discussion_r2349864005
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/BaseTaskGenerator.java:
##########
@@ -68,17 +68,35 @@ public long getTaskTimeoutMs() {
}
@Override
- public int getNumConcurrentTasksPerInstance() {
+ public int getNumConcurrentTasksPerInstance(String minionTag) {
String taskType = getTaskType();
- String configKey = taskType + MinionConstants.NUM_CONCURRENT_TASKS_PER_INSTANCE_KEY_SUFFIX;
- String configValue = _clusterInfoAccessor.getClusterConfig(configKey);
+
+ // Priority 1: Check minion tenant specific cluster config
+ if (minionTag != null) {
Review Comment:
Good suggestion, let me add a generic method and update getTaskTimeoutMs, getMaxAllowedSubTasksPerTask as well to use the generic method.
--
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] configure numConcurrentTasksPerInstance for minion tenant [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #16777:
URL: https://github.com/apache/pinot/pull/16777
--
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