jihoonson opened a new pull request #10732: URL: https://github.com/apache/druid/pull/10732
https://github.com/apache/druid/pull/10448 modified the `MonitorScheduler` to use `CronScheduler` instead of `ScheduledExecutorService`. This change looks good to me except that I'm not sure how well-tested `CronScheduler` is. This PR adds the previous `ScheduledExecutorService`-based `MonitorScheduler` back, and a new config, `druid.monitoring.schedulerClassName`, to determine what type of `MonitorScheduler` to use. This PR doesn't change the default `MonitorScheduler` as I find `CronScheduler` has a reasonable test coverage. However, if there is any unknown bug there, users can still use the old monitorScheduler. The new config is intentionally not documented as no one is supposed to touch it. However, it should be called out in the release notes. This PR additionally fixes 3 bugs in `MonitorScheduler`. - When an exception is thrown in `monitor.monitor()`, the behaviour has changed unexpectedly to stop the monitor. The monitor will ignore exceptions and continue working after this PR as it used to do. - There is a race condition between [when a scheduledFuture is set in a monitor](https://github.com/apache/druid/pull/10448/files#diff-e1da51fa67513b22b8110a76b29aa3c5cc8d00de418d7e5afc498c3bc3a8f107R178) and [when the scheduledFuture is used](https://github.com/apache/druid/pull/10448/files#diff-e1da51fa67513b22b8110a76b29aa3c5cc8d00de418d7e5afc498c3bc3a8f107R153). This will not likely happen in production since the first cronTask will be executed after the emitter period, but is possible in theory. - https://github.com/apache/druid/pull/10448 changed to use 64 threads for monitoring which seems an overkill to me. This PR changed it back to use a single thread. <hr> This PR has: - [x] been self-reviewed. - [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.) - [x] added documentation for new or modified features or behaviors. - [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links. - [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml) - [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader. - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met. - [ ] added integration tests. - [ ] been tested in a test Druid cluster. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org For additional commands, e-mail: commits-h...@druid.apache.org
