You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shounakmk219 (via GitHub)" <gi...@apache.org> on 2026/04/15 16:24:18 UTC
[PR] Track minion task generation logs based on id using mdc [pinot]
shounakmk219 opened a new pull request, #18215:
URL: https://github.com/apache/pinot/pull/18215
Use SLF4J MDC with a `genId` key formatted as `**<taskType>-<tableNameWithType>-<timestampMs>**`. This makes the ID self-descriptive: you can see what task type and table it relates to at a glance, and the timestamp ensures uniqueness.
**Placement**: MDC is set **only inside `PinotTaskManager`**. This keeps the change minimal and focused.
MDC is set at the **per-table/per-taskType level** inside the scheduling loop -- not at the outer `scheduleTasks()` level -- because `scheduleTasks()` can process multiple task types and tables in a single call.
_Note_ : not updating any log4j2 file as of now. Anyone who want to enable this you can change the pattern to something like `pattern="%d{HH:mm:ss.SSS} %p [%c{1}] [%t]%notEmpty{ [genId=%X{genId}]} %m%n"` in your log4j2 file.
--
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] Track minion task generation logs based on id using mdc [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #18215:
URL: https://github.com/apache/pinot/pull/18215#discussion_r3088001206
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -234,44 +236,49 @@ public synchronized Map<String, String> createTask(String taskType, String table
// responseMap holds the table to task name mapping.
Map<String, String> responseMap = new HashMap<>();
for (String tableNameWithType : tableNameWithTypes) {
- LOGGER.info("Trying to create tasks of type: {}, table: {}", taskType, tableNameWithType);
- TableConfig tableConfig = _pinotHelixResourceManager.getTableConfig(tableNameWithType);
- if (isExceedingResourceUtilizationLimits(tableNameWithType)) {
- continue;
- }
+ MDC.put(GEN_ID_KEY, getGenerationId(taskType, tableNameWithType));
Review Comment:
Is this needed in other places as well ?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -963,6 +970,7 @@ protected TaskSchedulingInfo scheduleTask(PinotTaskGenerator taskGenerator, List
Map<String, List<PinotTaskConfig>> minionInstanceTagToTaskConfigs = new HashMap<>();
for (TableConfig tableConfig : enabledTableConfigs) {
String tableName = tableConfig.getTableName();
+ MDC.put(GEN_ID_KEY, getGenerationId(taskType, tableName));
Review Comment:
Should this be inside try block
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -1068,6 +1078,10 @@ protected TaskSchedulingInfo scheduleTask(PinotTaskGenerator taskGenerator, List
return response.setScheduledTaskNames(submittedTaskNames);
}
+ protected static String getGenerationId(String taskType, String tableName) {
Review Comment:
Would it make sense to use UUID.randomUUID() ?
--
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] Track minion task generation logs based on id using mdc [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18215:
URL: https://github.com/apache/pinot/pull/18215#issuecomment-4254044998
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18215?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 55.28%. Comparing base ([`8e67864`](https://app.codecov.io/gh/apache/pinot/commit/8e678644a76ec12be9c07098cad777ebfb771d1a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f682ab7`](https://app.codecov.io/gh/apache/pinot/commit/f682ab74718f431da3406c39870f27a3a873ff60?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (8e67864) and HEAD (f682ab7). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (8e67864) | HEAD (f682ab7) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|java-11|4|0|
>|unittests2|2|0|
>|integration|5|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18215 +/- ##
============================================
- Coverage 63.33% 55.28% -8.06%
+ Complexity 1627 837 -790
============================================
Files 3238 2543 -695
Lines 197005 145962 -51043
Branches 30464 23471 -6993
============================================
- Hits 124782 80695 -44087
+ Misses 62226 58310 -3916
+ Partials 9997 6957 -3040
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18215/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/18215/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/18215/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/18215/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/18215/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/18215/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/18215/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.28% <ø> (-8.03%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18215/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.28% <ø> (-8.06%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18215/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.28% <ø> (-8.06%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18215/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.28% <ø> (-0.01%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18215/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/18215?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] Track minion task generation logs based on id using mdc [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #18215:
URL: https://github.com/apache/pinot/pull/18215#discussion_r3088098447
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -1068,6 +1078,10 @@ protected TaskSchedulingInfo scheduleTask(PinotTaskGenerator taskGenerator, List
return response.setScheduledTaskNames(submittedTaskNames);
}
+ protected static String getGenerationId(String taskType, String tableName) {
Review Comment:
Millis should be good imo, this ensures the id has all the info like task type, table name and around when it was running. We would never have concurrent task generations at same millisecond (central locking + synchronized schedule methods). Is there any specific concern with millis?
--
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] Track minion task generation logs based on id using mdc [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish merged PR #18215:
URL: https://github.com/apache/pinot/pull/18215
--
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] Track minion task generation logs based on id using mdc [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #18215:
URL: https://github.com/apache/pinot/pull/18215#discussion_r3088108752
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java:
##########
@@ -234,44 +236,49 @@ public synchronized Map<String, String> createTask(String taskType, String table
// responseMap holds the table to task name mapping.
Map<String, String> responseMap = new HashMap<>();
for (String tableNameWithType : tableNameWithTypes) {
- LOGGER.info("Trying to create tasks of type: {}, table: {}", taskType, tableNameWithType);
- TableConfig tableConfig = _pinotHelixResourceManager.getTableConfig(tableNameWithType);
- if (isExceedingResourceUtilizationLimits(tableNameWithType)) {
- continue;
- }
+ MDC.put(GEN_ID_KEY, getGenerationId(taskType, tableNameWithType));
Review Comment:
create (adhoc) and schedule (cron/manual trigger) are the only 2 entrypoint to generate tasks so it should cover all the flows. Are you suggesting we set mdc from the caller itself to track caller side logs as well?
--
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