amrishlal commented on a change in pull request #7174:
URL: https://github.com/apache/pinot/pull/7174#discussion_r692440213
##########
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:
Since this is a multi-threaded class, we want to have all the threads
that want to do the same thing (i.e run a task) pass through the same execution
chain using the same state variables. Doing this will avoid a lot of confusion
with respect to what thread is executing where and which state variables that
thread is using and how and where locks are being acquired and released. The
alternative (which I was trying earlier as Subbu mentioned) seemed to be
leading to a situation where we would not only have multiple threads, but also
multiple paths of execution (even if that path is just a few functions deep and
converges later on) and multiple references to the same state variable (base
class has _activePeriodicTaskProperties, do we pass that on the stack as well
to child classes when it is already available to all child classes. If not,
then why are we passing the "other" properties object on stack, etc).
Basically, it was bringing up too many issues and ending up with making to
o many assumptions; hence, I abandoned the approach.
The main problem here is that we need to replace the active
_activePeriodicTaskProperties object and that is really all that needs to be
done to make the same code work for all threads who want to do the same thing,
so the simplest way to do that seemed to be was to create a run(properties)
method to swap out _activePeriodicTaskProperties under lock and then call run()
method to let the rest of the code do what it is already doing.
##########
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:
Pending...
--
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
