You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/08/15 00:03:12 UTC
[PR] Shut down the task manager's cron scheduler when the controller stops [pinot]
Jackie-Jiang opened a new pull request, #19260:
URL: https://github.com/apache/pinot/pull/19260
## Summary
`PinotTaskManager` runs its per-table cron jobs on a Quartz scheduler of its own, created in the constructor and
started in `init()`. Nothing ever shut it down.
This is easy to miss because there are two Quartz schedulers in play. `PeriodicTaskScheduler` owns one and shuts it
down correctly in its `stop()`. The one `PinotTaskManager` owns is separate, and `cleanUpTask()` — the only shutdown
hook it participates in — just cleans up the task generators. So it kept firing after `stopPinotController()` had
closed the ZkClient through `_helixResourceManager.stop()`:
```
ERROR [JobRunShell] Job ... threw an unhandled Exception:
java.lang.IllegalStateException: ZkClient already closed!
at org.apache.helix.zookeeper.zkclient.ZkClient.retryUntilConnected(ZkClient.java:2166)
...
```
Quartz's `SimpleThreadPool` workers are not daemon threads either, so they outlived every controller stop. That
accumulates across controller restarts inside a single JVM, which is what integration tests do.
## Approach
`stopScheduler()` is added as the explicit counterpart to `init()`, and called from `stopPinotController()` right
after the periodic task scheduler stops — well before Helix is torn down.
It deliberately does *not* hang off `cleanUpTask()`, which would look like the natural hook. `cleanUpTask()` is also
used as a plain "reset the generators" operation on a live controller — see
`RealtimeToOfflineSegmentsMinionClusterIntegrationTest` — so attaching an irreversible scheduler shutdown to it would
be a trap for the next caller that does the same.
`shutdown(true)` waits for the jobs already in flight, so a task generation in progress is not cut off midway. This
matches what `PeriodicTaskScheduler` does for its own scheduler.
`stopScheduler()` is a no-op when the cron scheduler is disabled, which is the default
(`controller.task.scheduler.enabled`), so nothing changes for controllers that never start one.
## Coverage
`PinotTaskManagerStatelessTest#testSchedulerShutDownWithController` starts a controller with the scheduler enabled,
stops it, and asserts the scheduler is shut down. It fails on `master` and passes with this change.
--
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] Shut down the task manager's cron scheduler when the controller stops [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19260:
URL: https://github.com/apache/pinot/pull/19260
--
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] Shut down the task manager's cron scheduler when the controller stops [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19260:
URL: https://github.com/apache/pinot/pull/19260#issuecomment-5299601868
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19260?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 57.72%. Comparing base ([`388bc64`](https://app.codecov.io/gh/apache/pinot/commit/388bc64d5c89265ba0b0ab70c2b59af81d3a7862?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`35b73d2`](https://app.codecov.io/gh/apache/pinot/commit/35b73d2beda65864176b95db9d7b5e7446508f6b?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 (388bc64) and HEAD (35b73d2). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (388bc64) | HEAD (35b73d2) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|lane-a|2|0|
>|integration|4|0|
>|integration1|2|0|
>|lane-b|2|0|
>|integration2|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19260 +/- ##
============================================
- Coverage 66.97% 57.72% -9.26%
+ Complexity 1423 1 -1422
============================================
Files 3453 2656 -797
Lines 218949 159214 -59735
Branches 34805 26113 -8692
============================================
- Hits 146648 91907 -54741
+ Misses 60585 59506 -1079
+ Partials 11716 7801 -3915
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19260/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/19260/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/19260/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/19260/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/19260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.72% <ø> (-9.26%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19260/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/19260/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/19260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.72% <ø> (-9.26%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.72% <ø> (-9.25%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.72% <ø> (-0.02%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19260/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/19260?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