Jackie-Jiang commented on a change in pull request #7174:
URL: https://github.com/apache/pinot/pull/7174#discussion_r692568185
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/periodictask/BasePeriodicTask.java
##########
@@ -29,21 +33,31 @@
@ThreadSafe
public abstract class BasePeriodicTask implements PeriodicTask {
private static final Logger LOGGER =
LoggerFactory.getLogger(BasePeriodicTask.class);
+ private static final String DEFAULT_REQUEST_ID = "auto";
// Wait for at most 30 seconds while calling stop() for task to terminate
private static final long MAX_PERIODIC_TASK_STOP_TIME_MILLIS = 30_000L;
protected final String _taskName;
protected final long _intervalInSeconds;
protected final long _initialDelayInSeconds;
+ protected final ReentrantLock _runLock;
private volatile boolean _started;
private volatile boolean _running;
+ // Properties that task may use during execution. null by default.
+ protected Properties _activePeriodicTaskProperties;
Review comment:
Logically they are 2 different tasks: one periodic trigger to process
all tables lead by the controller; the other one adhoc to process based on the
custom properties. The second one might only need to process one table, why
mixing them and force the second one to scan potentially thousands of tables?
Also, I don't agree that for multi-threaded class, we want to follow the
same execution chain and mixing the logics. It won't really help with the race
condition handling, and more complicated logic within a single method can lead
to more bugs.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
