Tidy-up TimedRunnable #41785
Closed
Tidy-up TimedRunnable #41785
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Asserts that taskExecutionNanos is _strictly_ positive - Removes impossible `startTimeNanos == -1` condition - Implements `toString()` This last point is because there have been two recent failures of PR builds of the following form: ``` 2> KKN 22, 2019 11:40:12 PM com.carrotsearch.randomizedtesting.RandomizedRunner$QueueUncaughtExceptionsHandler uncaughtException 2> WARNING: Uncaught exception in thread: Thread[elasticsearch[mock_ node][search][T#1],5,TGRP-NodeTests] 2> java.lang.AssertionError: expected task to always take longer than 0 nanoseconds, got: -1 2> at __randomizedtesting.SeedInfo.seed([99E328AA85EDFC3A]:0) 2> at org.elasticsearch.common.util.concurrent.QueueResizingEsThreadPoolExecutor.afterExecute(QueueResizingEsThreadPoolExecutor.java:154) 2> at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1131) 2> at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) 2> at java.base/java.lang.Thread.run(Thread.java:834) ``` See for instance https://scans.gradle.com/s/itejjhgbqu4uo/console-log?task=:server:test I cannot immediately see how this can happen, but cannot deduce much from this failure message, so have added more details to help a future investigation.
DaveCTurner
added
>non-issue
:Core/Infra/Core
Collaborator
|
Pinging @elastic/es-core-infra |
DaveCTurner
commented
May 3, 2019
| 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 : |
Contributor
Author
There was a problem hiding this comment.
@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.
dakrone
approved these changes
May 21, 2019
Member
dakrone
left a comment
dakrone
left a comment
There was a problem hiding this comment.
LGTM, I am not sure about the exception throwing you mentioned either, regardless, I think this will provide additional feedback if it occurs again.
Contributor
Author
|
Superseded by #41810 it seems. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Asserts that taskExecutionNanos is strictly positive
Removes impossible
startTimeNanos == -1conditionImplements
toString()This last point is because there have been two recent failures of PR builds of
the following form:
See for instance
https://scans.gradle.com/s/itejjhgbqu4uo/console-log?task=:server:test
I cannot immediately see how this can happen, but cannot deduce much from this
failure message, so have added more details to help a future investigation.