xiangfu0 commented on code in PR #18256:
URL: https://github.com/apache/pinot/pull/18256#discussion_r3315358053
##########
pinot-core/src/main/java/org/apache/pinot/core/periodictask/PeriodicTaskScheduler.java:
##########
@@ -57,41 +67,92 @@ public void init(List<PeriodicTask> periodicTasks) {
/**
* Starts scheduling periodic tasks.
+ * It uses the cron expression if provided, if not, it falls back to the
default fixed delay scheduling.
+ *
*/
public synchronized void start() {
if (_executorService != null) {
LOGGER.warn("Periodic task scheduler already started");
+ return;
}
+ Preconditions.checkState(_periodicTasks != null,
+ "Periodic task scheduler has not been initialized");
if (_periodicTasks.isEmpty()) {
LOGGER.warn("No periodic task scheduled");
- } else {
- Collection<PeriodicTask> periodicTasks = _periodicTasks.values();
- LOGGER.info("Starting periodic task scheduler with tasks: {}",
periodicTasks);
- _executorService =
Executors.newScheduledThreadPool(_periodicTasks.size());
+ return;
+ }
+
+ Collection<PeriodicTask> periodicTasks = _periodicTasks.values();
+ LOGGER.info("Starting periodic task scheduler with tasks: {}",
periodicTasks);
+
+ for (PeriodicTask task : periodicTasks) {
+ String cron = task.getCronExpression();
+ if (cron != null && !cron.trim().isEmpty() &&
!CronExpression.isValidExpression(cron)) {
+ throw new IllegalArgumentException(
+ String.format("Invalid CRON expression '%s' for task '%s'. Halting
controller startup.",
+ cron, task.getTaskName())
+ );
+ }
+ }
+
+ boolean hasCronTasks = periodicTasks.stream()
+ .map(PeriodicTask::getCronExpression)
+ .anyMatch(cron -> cron != null && !cron.trim().isEmpty());
+
+ if (hasCronTasks) {
+ try {
+ try {
+ _scheduler = new StdSchedulerFactory().getScheduler();
Review Comment:
Issue: The update changed getDefaultScheduler() to new
StdSchedulerFactory().getScheduler(), but that still uses Quartz’s default
scheduler repository. If controller.task.scheduler.enabled=true,
PinotTaskManager and the new controller periodic cron scheduler can still share
the same Quartz scheduler while both components treat it as privately owned.
Risk: Table-task cron jobs and controller periodic cron jobs can share
lifecycle and namespace unexpectedly. PeriodicTaskScheduler.stop() can shut
down the scheduler used by PinotTaskManager, and PinotTaskManager still does
broad scheduler scans/deletes using GroupMatcher.anyJobGroup(). Blast radius is
controller cron/minion scheduling, not wire format or query path.
Suggested fix: Either create a dedicated named scheduler for controller
periodic tasks, or introduce an explicit shared scheduler owner with distinct
job groups and no broad cross-owner operations. Add a regression test with
minion cron scheduling enabled plus at least one controller periodic cron task.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
