You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/10/02 16:59:08 UTC
[GitHub] [activemq-artemis] franz1981 opened a new pull request #3287: ARTEMIS-2926 Scheduled sub-millis duration tasks can be skipped
franz1981 opened a new pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287
https://issues.apache.org/jira/browse/ARTEMIS-2926
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512866481
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -265,6 +270,9 @@ public void stop() {
if (future != null) {
future.cancel(false);
future = null;
+ // Replace te existing one: a new periodic task or any new delay after stop
+ // won't interact with the previously running ones
+ this.bookedForRunning = new AtomicBoolean(false);
Review comment:
I think we are missing a `isStarted` field somewhere....
We are not correctly tracking when `start`/`stop` are called
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717700492
@clebertsuconic I'm now investigating, but is strange: the failures seems not related to my change, indeed I'm getting failures on tests that are not using the scheduled component...
I'm trying to remove the latest commits before mine to check if that's the cause of the failure...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512832724
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,15 +181,27 @@ public ClassLoader run() {
}
- public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
Review comment:
Comment needs updated
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -265,6 +270,9 @@ public void stop() {
if (future != null) {
future.cancel(false);
future = null;
+ // Replace te existing one: a new periodic task or any new delay after stop
+ // won't interact with the previously running ones
+ this.bookedForRunning = new AtomicBoolean(false);
Review comment:
The comment doesnt entirely fit if this is inside the null check, does it? It wont apply for a 'new delay' since the future doesnt exist for such 'on demand' usage.
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -265,6 +270,9 @@ public void stop() {
if (future != null) {
future.cancel(false);
future = null;
+ // Replace te existing one: a new periodic task or any new delay after stop
Review comment:
"the"
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -287,35 +295,47 @@ private void restartIfNeeded() {
}
}
- final Runnable runForExecutor = new Runnable() {
- @Override
- public void run() {
- if (onDemand && delayed.get() > 0) {
- delayed.decrementAndGet();
- }
+ private void runForExecutor(AtomicBoolean booked) {
+ // It unblocks:
+ // - a new delay request
+ // - next periodic run request (in case of executor != null)
+ // Although tempting, don't move this one after ActiveMQScheduledComponent.this.run():
+ // - it can cause "delay" to change semantic ie a racing delay while finished executing the task, won't succeed
+ // - it won't prevent "slow tasks" to accumulate, because slowness cannot be measured inside running method;
+ // it just cause skipping runs for perfectly timed executions too
+ // Re the code as it is: if the next periodic run has been able to request a new round while
+ // the previous one hasn't yet started, it makes sense to skip it, because its job will be executed by
+ // the old one (yet to start).
Review comment:
Not sure this bit of comment is in the right place, or if so could do with updating. This code here doesnt skip anything, as the comment reads like. It instead resets the flag used by other areas to skip.
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -287,35 +295,47 @@ private void restartIfNeeded() {
}
}
- final Runnable runForExecutor = new Runnable() {
- @Override
- public void run() {
- if (onDemand && delayed.get() > 0) {
- delayed.decrementAndGet();
- }
+ private void runForExecutor(AtomicBoolean booked) {
+ // It unblocks:
+ // - a new delay request
+ // - next periodic run request (in case of executor != null)
+ // Although tempting, don't move this one after ActiveMQScheduledComponent.this.run():
+ // - it can cause "delay" to change semantic ie a racing delay while finished executing the task, won't succeed
+ // - it won't prevent "slow tasks" to accumulate, because slowness cannot be measured inside running method;
+ // it just cause skipping runs for perfectly timed executions too
+ // Re the code as it is: if the next periodic run has been able to request a new round while
+ // the previous one hasn't yet started, it makes sense to skip it, because its job will be executed by
+ // the old one (yet to start).
+ boolean alwaysTrue = booked.compareAndSet(true, false);
+ assert alwaysTrue;
+ ActiveMQScheduledComponent.this.run();
+ }
- if (!onDemand && lastTime > 0) {
- if (System.currentTimeMillis() - lastTime < millisecondsPeriod) {
- logger.trace("Execution ignored due to too many simultaneous executions, probably a previous delayed execution");
- return;
+ private void bookedRunForScheduler(AtomicBoolean booked) {
+ if (executor != null) {
+ assert booked.get();
Review comment:
assert is in both legs, move outside if
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717623140
@franz1981 I have seen the results of the test suite on our CI, and in both cases the test suite did not complete.
I don't think it is safe to merge this ATM.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717082441
@gemmellr
A couple of observations on the changes on be3c193 on https://github.com/apache/activemq-artemis/blob/be3c193f73775b961f855e6d935c442e28a14875/artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java#L324-L331
I've chosen on purpose to allow a racing booking request (can be either from a `delay` or the next periodic task submission on `executor`) to happen before instead of on completion of `ActiveMQScheduledComponent.this.run()`:
- because of the expected behaviour of `delay`:
eg `JDBCJournalImpl` submit some records and call `syncTimer.delay()`; it means that if a pending task hasn't yet hit`bookedForRunning.compareAndSet(true, false)`, the delay request will fail, leaving some records to not be picked up and processed later
- because of the uselessness (due to how a JVM work?) to detect a "slow" (ie which execution time is > period) pending task (see https://issues.apache.org/jira/browse/ARTEMIS-2926?focusedCommentId=17212908&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17212908): TLDR if `bookedForRunning.compareAndSet(true, false)` takes long time to be hit it doesn't mean that the task hasn't been prompty executed/is slow -> skipping the next period task just means we're slowing down rate!
I've (by default) added a guard to save tasks to accumulate on the `executor` or on the `scheduledExecutorService` (due to `delay`), but this can be disabled for specific cases.
Although seems dangerous this guard is fine because a delay to start the current task just means that is pointless to submit new ones, given that any pending jobs will be picked by the current one when it will start.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717773167
I've added a new commit but seems to create some trouble: I'll work today to fix what's wrong here...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512695885
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
+ bookedForRunning.set(false);
+ }
+ throw e;
+ }
}
}
public long getPeriod() {
- return period;
+ return timeUnit.convert(nsPeriod, TimeUnit.NANOSECONDS);
Review comment:
Actually, I see there now is a getTimeUnit, so I think this change needs effectively unwound...it should return the same value as passed in.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512696109
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -265,6 +291,14 @@ public void stop() {
if (future != null) {
future.cancel(false);
future = null;
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning != null) {
+ // this is trying to fast-fail any running task or pending one:
+ bookedForRunning.compareAndSet(false, true);
Review comment:
> Have you checked if there is there any impact of this behaviour change? If not, might it be worth omitting this bit, and just leaving the reassignment below for now?
It's the safer choice here so will do as you suggest and simplify the rest of the code (although "simplify" is rather optimistic here XD )
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512714310
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
+ bookedForRunning.set(false);
+ }
+ throw e;
+ }
}
}
public long getPeriod() {
- return period;
+ return timeUnit.convert(nsPeriod, TimeUnit.NANOSECONDS);
Review comment:
TBH given that we don't measure time anymore it's fine :)
It means we will get a simpler code there although not a big fan of allowing mutability of 2 separate things that are related to each others (period and time unit)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717871830
@clebertsuconic It seems that I was missing to correctly stop :)
The CI now is green :+1:
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512717430
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
Review comment:
And indeed it was, good eyes!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3287: ARTEMIS-2926 Scheduled sub-millis duration tasks can be skipped
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-707028383
I think the changes seem ok, but I think perhaps the PR overlooks another simpler and more important behaviour that may be leading to the observed issue?
The given period at construction of the scheduled tasks is documented as "the delay between the termination of one execution and the start of the next". Thats unsurprisingly consistent with the behaviour of scheduledExecutorService.scheduleWithFixedDelay(), which is whats used by the 'not onDemand' instances of the scheduled tasks. However, the tasks dont actually run in the scheduledExecutorService thread if the additional executor is given during construction. If the second executor is given, the scheduled task is just offloaded by the scheduledExecutorService for execution on the provided executor and entirely forgotten about. That seems like it could be the core of the observed issue to me?
The above means there is no further tracking by the scheduler of when the task actually runs or how long the given task takes, meaning the periodic contract is somewhat lost at that point forward. Consider a situation:
1. Say there is a backlog of existing (related or unrelated) things for the executor still to run, so a new 'scheduled offloaded' task may not run for a little while until that is processed. Or instead say that thread scheduling means the second executor doesnt immediately get to executing the task. Whatever the reason, something means there is a small delay, but eventually the task does run.
2. A second task instance comes along from the scheduledExecutorService at some point, very closely after the configured period since it isnt affected by actual execution of the task, which gets offloaded. Maybe now there isnt any or as much backlog on the second executor, or theres a better thread scheduling environment, and this second task may get run relatively quicker than the prior instance actualy did.
3. Due to the 'lastTime' tracking occuring within the task itself, on the second executor, this second task instance which was offloaded by the scheduledExecutorService at its precise period, will now be observed to have occurred within the configured period of the previous tasks 'lstTime' and so get skipped.
4. This means nothing happened, and wont until the scheduledExecutorService comes along after a 3rd period and offloads the task another time, by which point approx double the expected period has elapsed and the task actually executes. Rinse and repeat this process over and over.
If the second executor is provided, its actual execution + 'lastTime' period checks are essentially happening independently of the scheduling, and it seems like the scheduledExecutorService is trying to somewhat blindly throw tasks over a wall such that they land at the right time and actually get to run as opposed to skipping and waiting for next time.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717700492
@clebertsuconic I'm now investigating, but is strange: the failures seems not related to my change, indeed I'm getting failures on tests that are not using the scheduled component...
I'm trying removing the latest commits before mine to check if that's the cause of the failure...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512694529
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -60,41 +58,58 @@
* @param checkPeriod the delay between the termination of one execution and the start of the next
* @param timeUnit the time unit of the {@code initialDelay} and {@code checkPeriod} parameters
* @param onDemand if {@code true} the task won't be scheduled on {@link #start()}, {@code false} otherwise
+ * @param overBooking if {@code true} {@link #delay()} executions and period ones are allowed to accumulate, {@code false} otherwise. Default is {@code false}.
*/
public ActiveMQScheduledComponent(ScheduledExecutorService scheduledExecutorService,
Executor executor,
long initialDelay,
long checkPeriod,
TimeUnit timeUnit,
- boolean onDemand) {
+ boolean onDemand,
+ boolean overBooking) {
Review comment:
> I would remove the overbooking, eliminate the possibility of null for the atomic, and simplify the rest of the code accordingly. It will still be plenty complicated :)
+100 agree: I was thinking that was needed for the JDBC lease lock, but effectively isn't needed, I'm removing it
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512692776
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
+ bookedForRunning.set(false);
+ }
+ throw e;
+ }
}
}
public long getPeriod() {
- return period;
+ return timeUnit.convert(nsPeriod, TimeUnit.NANOSECONDS);
Review comment:
Good idea: I remember I've checked every possible caller of it, but...without tests we can easily get regressions and I could have missed something :+1:
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512714310
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
+ bookedForRunning.set(false);
+ }
+ throw e;
+ }
}
}
public long getPeriod() {
- return period;
+ return timeUnit.convert(nsPeriod, TimeUnit.NANOSECONDS);
Review comment:
TBH given that we don't measure time anymore it's fine :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717082441
@gemmellr
A couple of observations on the changes of be3c193 on https://github.com/apache/activemq-artemis/blob/be3c193f73775b961f855e6d935c442e28a14875/artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java#L324-L331
I've chosen on purpose to allow a racing booking request (can be either from a `delay` or the next periodic task submission on `executor`) to happen before instead of on completion of `ActiveMQScheduledComponent.this.run()`:
- because of the expected behaviour of `delay`:
eg `JDBCJournalImpl` submit some records and call `syncTimer.delay()`; it means that if a pending task hasn't yet hit`bookedForRunning.compareAndSet(true, false)`, the delay request will fail, leaving some records to not be picked up and processed later
- because of the uselessness (due to how a JVM work?) to detect a "slow" (ie which execution time is > period) pending task (see https://issues.apache.org/jira/browse/ARTEMIS-2926?focusedCommentId=17212908&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17212908): TLDR if `bookedForRunning.compareAndSet(true, false)` takes long time to be hit it doesn't mean that the task hasn't been prompty executed/is slow -> skipping the next period task just means we're slowing down rate!
I've (by default) added a guard to save tasks to accumulate on the `executor` or on the `scheduledExecutorService` (due to `delay`), but this can be disabled for specific cases.
Although seems dangerous this guard is fine because a delay to start the current task just means that is pointless to submit new ones, given that any pending jobs will be picked by the current one when it will start.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
gemmellr commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512555293
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
+ bookedForRunning.set(false);
+ }
+ throw e;
+ }
}
}
public long getPeriod() {
- return period;
+ return timeUnit.convert(nsPeriod, TimeUnit.NANOSECONDS);
Review comment:
May not matter, but did you check if this behaviour change has any impact?
(The units were not taken into account originally, whatever checkPeriod value was passed in at construction was passed back out unchanged without considering the timeUnit...while now it will be converted from nanos into the given timeUnit)
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
+ bookedForRunning.set(false);
+ }
+ throw e;
+ }
}
}
public long getPeriod() {
- return period;
+ return timeUnit.convert(nsPeriod, TimeUnit.NANOSECONDS);
}
public synchronized ActiveMQScheduledComponent setPeriod(long period) {
- this.period = period;
+ this.nsPeriod = TimeUnit.NANOSECONDS.convert(period, timeUnit);
restartIfNeeded();
return this;
}
public synchronized ActiveMQScheduledComponent setPeriod(long period, TimeUnit unit) {
- this.period = period;
this.timeUnit = unit;
+ this.nsPeriod = TimeUnit.NANOSECONDS.convert(period, unit);
restartIfNeeded();
return this;
}
public long getInitialDelay() {
- return initialDelay;
+ return timeUnit.convert(initialNsDelay, TimeUnit.NANOSECONDS);
Review comment:
Same question about unit conversion as earlier.
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -287,35 +321,63 @@ private void restartIfNeeded() {
}
}
- final Runnable runForExecutor = new Runnable() {
- @Override
- public void run() {
- if (onDemand && delayed.get() > 0) {
- delayed.decrementAndGet();
+ private void runForExecutor(AtomicBoolean bookedForRunning) {
+ if (bookedForRunning != null) {
+ if (!bookedForRunning.compareAndSet(true, false)) {
+ // this can happen only if there is a racing stop
+ return;
}
+ }
+ ActiveMQScheduledComponent.this.run();
+ }
- if (!onDemand && lastTime > 0) {
- if (System.currentTimeMillis() - lastTime < millisecondsPeriod) {
- logger.trace("Execution ignored due to too many simultaneous executions, probably a previous delayed execution");
+ private void delayedRunForScheduler(AtomicBoolean bookedForRunning) {
+ if (executor != null) {
+ if (bookedForRunning != null) {
+ if (!bookedForRunning.get()) {
+ // this can happen only if there is a racing stop
return;
}
}
-
- lastTime = System.currentTimeMillis();
-
- ActiveMQScheduledComponent.this.run();
+ try {
+ executor.execute(() -> runForExecutor(bookedForRunning));
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning != null) {
+ bookedForRunning.set(false);
+ }
+ throw e;
+ }
+ } else {
+ runForExecutor(bookedForRunning);
}
- };
-
- final Runnable runForScheduler = new Runnable() {
- @Override
- public void run() {
- if (executor != null) {
- executor.execute(runForExecutor);
- } else {
- runForExecutor.run();
+ }
+
+ private void runForScheduler(AtomicBoolean bookedForRunning) {
+ if (executor != null) {
+ if (bookedForRunning != null && !bookedForRunning.compareAndSet(false, true)) {
Review comment:
Both of the 'if executor != null' legs do this check first, I think it would be simpler to move it outside the if and do it once atthe start. The descriptions dont differ enough to justify both IMO, so its more rather than less complicated/confusing having both.
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
Review comment:
Might be simpler to follow without the variable shadowing (here and in the called methods).
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -60,41 +58,58 @@
* @param checkPeriod the delay between the termination of one execution and the start of the next
* @param timeUnit the time unit of the {@code initialDelay} and {@code checkPeriod} parameters
* @param onDemand if {@code true} the task won't be scheduled on {@link #start()}, {@code false} otherwise
+ * @param overBooking if {@code true} {@link #delay()} executions and period ones are allowed to accumulate, {@code false} otherwise. Default is {@code false}.
*/
public ActiveMQScheduledComponent(ScheduledExecutorService scheduledExecutorService,
Executor executor,
long initialDelay,
long checkPeriod,
TimeUnit timeUnit,
- boolean onDemand) {
+ boolean onDemand,
+ boolean overBooking) {
Review comment:
If this isnt used I would just remove it. It makes the actually-used code more complicated to understand than it needs to be. Ignoring things that take longer than 'period' to run, the previous/existing stuff couldnt overbook-without-end (was capped at ~10 outstanding 'on demands'), while this seemingly could, so its in fact worse in that regard.
I would remove the overbooking, eliminate the possibility of null for the atomic, and simplify the rest of the code accordingly. It will still be plenty complicated :)
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning == null || bookedForRunning.compareAndSet(false, true)) {
+ try {
+ scheduledExecutorService.schedule(() -> delayedRunForScheduler(bookedForRunning), nsPeriod, TimeUnit.NANOSECONDS);
+ } catch (RejectedExecutionException e) {
+ if (bookedForRunning == null) {
Review comment:
Guessing that was meant to be != null? EDIT: as later commenting, think the null check should be eliminated through simplification.
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -265,6 +291,14 @@ public void stop() {
if (future != null) {
future.cancel(false);
future = null;
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
+ if (bookedForRunning != null) {
+ // this is trying to fast-fail any running task or pending one:
+ bookedForRunning.compareAndSet(false, true);
Review comment:
This goes at odds with the prior behaviour, where no attempt was made to stop the tasks already handed to the executor or the final scheduling already being in the process of either handing off another to the executor or running it directly on the scheduledExecutorService. The latter is still true to an extent, the future.cancel(false) remains above, but this new bit will ensure it doesnt actually do anything unless it already passed the gate and starts the 'ActiveMQScheduledComponent.this.run();'.
Have you checked if there is there any impact of this behaviour change? If not, might it be worth omitting this bit, and just leaving the reassignment below for now?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512716505
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,41 +202,53 @@ public ClassLoader run() {
}
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in {@link #getPeriod()}.<br>
+ */
public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period, period * value), timeUnit);
+ final AtomicBoolean bookedForRunning = this.bookedForRunning;
Review comment:
Sorry, bad habit of mine to force using the local variable; they're funcionally the same so always complex to find an appropriate name (!= `localBookedForRunning`)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#issuecomment-717082441
@gemmellr
A couple of observations on the changes on be3c193 on https://github.com/apache/activemq-artemis/blob/be3c193f73775b961f855e6d935c442e28a14875/artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java#L324-L331
I've chosen on purpose to allow a racing booking request (can be either from a `delay` or the next periodic task submission on `executor`) to happen before instead of on completion of `ActiveMQScheduledComponent.this.run()`:
- because of the expected behaviour of `delay`:
eg `JDBCJournalImpl` submit some records and call `syncTimer.delay()`; it means that if a pending task hasn't yet hit`bookedForRunning.compareAndSet(true, false)`, the delay request will fail, leaving some records to not be picked up and processed later
- because of the uselessness (due to how a JVM work?) to detect a "slow" (ie which execution time is > period) pending task (see https://issues.apache.org/jira/browse/ARTEMIS-2926?focusedCommentId=17212908&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17212908): TLDR if `bookedForRunning.compareAndSet(true, false)` takes long time to be hit it doesn't mean that the task hasn't been prompty executed/is slow -> skipping the next period task just means we're slowing down rate!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3287: ARTEMIS-2926 Scheduled task executions are skipped randomly
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512860312
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -287,35 +295,47 @@ private void restartIfNeeded() {
}
}
- final Runnable runForExecutor = new Runnable() {
- @Override
- public void run() {
- if (onDemand && delayed.get() > 0) {
- delayed.decrementAndGet();
- }
+ private void runForExecutor(AtomicBoolean booked) {
+ // It unblocks:
+ // - a new delay request
+ // - next periodic run request (in case of executor != null)
+ // Although tempting, don't move this one after ActiveMQScheduledComponent.this.run():
+ // - it can cause "delay" to change semantic ie a racing delay while finished executing the task, won't succeed
+ // - it won't prevent "slow tasks" to accumulate, because slowness cannot be measured inside running method;
+ // it just cause skipping runs for perfectly timed executions too
+ // Re the code as it is: if the next periodic run has been able to request a new round while
+ // the previous one hasn't yet started, it makes sense to skip it, because its job will be executed by
+ // the old one (yet to start).
Review comment:
Yep effectively is not the right place for this bits of code, but is more the overall logic...I can put it in the header of the class docs!
----------------------------------------------------------------
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:
users@infra.apache.org