Handle interruption in RetryDriver and TaskRunner #18964
Merged
Conversation
This file contains 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
pettyjamesm
force-pushed
the
improve-interrupted-handling
branch
from
8df783b to
e05a499
Compare
ajaygeorge
reviewed
Jan 31, 2023
presto-hive/src/main/java/com/facebook/presto/hive/s3/PrestoS3FileSystem.java
Outdated
Show resolved
Hide resolved
presto-hive-metastore/src/main/java/com/facebook/presto/hive/RetryDriver.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/execution/executor/TaskExecutor.java
Show resolved
Hide resolved
pettyjamesm
force-pushed
the
improve-interrupted-handling
branch
2 times, most recently
from
2285985 to
394f561
Compare
rschlussel
approved these changes
Feb 2, 2023
When a thread is interrupted during an S3 client operation the SDK client internally catches the InterruptedException, sets the thread back to interrupted, and throws an AbortedException instead of InterruptedException. When this occurs, the RetryDriver should stop attempting retries. This change adds AbortedException to the stopOn list for all retry drivers in PrestoS3FileSystem and S3SelectLineRecordReader
Stop attempting retries in Hive's RetryDriver if an InterruptedException is caught or when the current thread is interrupted and immediately throw the current exception. Otherwise, RetryDriver might interfere with drivers terminating in a timely manner after tasks are terminated via a cancel, failure, or abort.
Drivers may leave the TaskRunner thread's interrupt flag set during the course of processing, but doing so should not result in the TaskRunner terminating its own processing loop until the TaskExecutor is closed. Instead of allowing the interrupt to terminate the current task runner's loop and re-creating a new runner to replace the interrupted one, we can clear the interrupt flag after each iteration. Otherwise, TaskRunners that were interrupted would have to replace themselves and end up creating unnecessary threads in the cached threadpool executor in the process.
pettyjamesm
force-pushed
the
improve-interrupted-handling
branch
from
394f561 to
db99445
Compare
| @@ -139,6 +139,11 @@ public <V> V run(String callableName, Callable<V> callable) | |||
| return callable.call(); | |||
| } | |||
| catch (Exception e) { | |||
| // Immediately stop retry attempts once an interrupt has been received | |||
| if (e instanceof InterruptedException || Thread.currentThread().isInterrupted()) { | |||
There was a problem hiding this comment.
Sorry for the late comment, Checking the isInterrupted will clear the interrupt flag on the thread, is that expected ? I am not sure who is responsible for clearing the interrupt flag in the Presto codebase.
Sleep on line 163, tries to rightly set the interrupted flag.
There was a problem hiding this comment.
My bad, isInterrupted does not clear the flag, so it is good. but on handling the interruptedException, there are still differences.
There was a problem hiding this comment.
We’re rethrowing InterruptedException (or whatever other exception was thrown if the current thread is interrupted) but not “handing” it other than to ensure that we stop retrying and let the exception propagate.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Cross port of trinodb/trino#15803 with one additional commit to fix handling of S3 client exceptions when interrupted. At a high level, this PR:
AbortedExceptionto the list ofRetryDriver#stopOnexceptions for all S3 client operations inPrestoS3FileSystemandS3SelectLineRecordReader. When the S3 client receives anInterruptedExceptioninternally during an API call operation, it re-sets the current threads interrupted flag, but re-throws anAbortedException. When this occurs, the retry driver should stop attempting retries.RetryDriverexception handling to stop retries when anInterruptedExceptionis caught orThread.currentThread().isInterrupted(). Failing to do so could result in drivers that were interrupted as part of task cancellation running significantly longer than necessary by proceeding to retry / backoff instead of exiting.TaskRunnerto continue processing new splits instead of terminating when the current thread was interrupted as a result of task cancellation interrupting the current driver being processed withoutTaskExecutorhaving been shut down. Before this change, those interruptions would cause theTaskRunnerto stop processing new splits and submit a newTaskRunnerinto the cached threadpool executor, which could needlessly create new worker threads.