Skip to content

Tidy-up TimedRunnable #41785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Tidy-up TimedRunnable #41785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2201444
Tidy-up TimedRunnable
DaveCTurner May 3, 2019
327244e
Allow for an exception from a wrapper
DaveCTurner May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions ...main/java/org/elasticsearch/common/util/concurrent/QueueResizingEsThreadPoolExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,14 @@ protected void afterExecute(Runnable r, Throwable t) {
final long totalNanos = totalTaskNanos.addAndGet(taskNanos);

final long taskExecutionNanos = timedRunnable.getTotalExecutionNanos();
assert taskExecutionNanos >= 0 : "expected task to always take longer than 0 nanoseconds, got: " + taskExecutionNanos;
executionEWMA.addValue(taskExecutionNanos);
if (taskExecutionNanos > 0) {
executionEWMA.addValue(taskExecutionNanos);
} else {
// wrapper (e.g. ContextPreservingAbstractRunnable) threw an exception before being able to run this task
assert taskExecutionNanos == -1 && timedRunnable.isStarted() == false && t != null :
Copy link
Contributor Author

@DaveCTurner DaveCTurner May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dakrone I'm not really sure about this, because the exception that ContextPreservingAbstractRunnable throws when shut down is re-thrown after we get here here, and goes uncaught, so the tests that are failing here look like they'd fail anyway. I'm also unsure whether we want to continue with the rest of the method, or bail out, or something else entirely.

"expected task exceptionally never to have started, but got: "
+ taskExecutionNanos + " for " + timedRunnable + " with exception [" + t + "]";
}

if (taskCount.incrementAndGet() == this.tasksPerFrame) {
final long endTimeNs = System.nanoTime();
Expand Down
16 changes: 13 additions & 3 deletions server/src/main/java/org/elasticsearch/common/util/concurrent/TimedRunnable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class TimedRunnable extends AbstractRunnable implements WrappedRunnable {
@Override
public void doRun() {
try {
assert finishTimeNanos == -1 : "task already run: finishTimeNanos=" + finishTimeNanos + " for " + this;
startTimeNanos = System.nanoTime();
original.run();
} finally {
Expand Down Expand Up @@ -82,7 +83,6 @@ public boolean isForceExecution() {
*/
long getTotalNanos() {
if (finishTimeNanos == -1) {
// There must have been an exception thrown, the total time is unknown (-1)
return -1;
}
return Math.max(finishTimeNanos - creationTimeNanos, 1);
Expand All @@ -93,8 +93,7 @@ long getTotalNanos() {
* If the task is still running or has not yet been run, returns -1.
*/
long getTotalExecutionNanos() {
if (startTimeNanos == -1 || finishTimeNanos == -1) {
// There must have been an exception thrown, the total time is unknown (-1)
if (finishTimeNanos == -1) {
return -1;
}
return Math.max(finishTimeNanos - startTimeNanos, 1);
Expand All @@ -105,4 +104,15 @@ public Runnable unwrap() {
return original;
}

@Override
public String toString() {
return "TimedRunnable{" +
"original=" + original +
'}';
}

// used by assertions only
public boolean isStarted() {
return startTimeNanos != 0;
}
}