[LIVY-786] Interrupt interpreter threads when cancelling statements#495
[LIVY-786] Interrupt interpreter threads when cancelling statements#495ArnavBalyan wants to merge 5 commits intoapache:masterfrom
Conversation
ffd922a to
922215d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #495 +/- ##
=============================================
- Coverage 68.38% 54.35% -14.03%
+ Complexity 1199 863 -336
=============================================
Files 106 106
Lines 6711 6717 +6
Branches 831 831
=============================================
- Hits 4589 3651 -938
- Misses 1657 2621 +964
+ Partials 465 445 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request enhances the statement cancellation mechanism in Apache Livy by adding thread interruption capabilities to cancel driver code that doesn't involve Spark jobs. Previously, the cancel API only cancelled Spark jobs but couldn't interrupt code running directly on the driver thread (e.g., Thread.sleep()).
Key Changes:
- Added thread tracking using a
ConcurrentHashMapto map statement IDs to their executing threads - Implemented thread interruption during statement cancellation to terminate non-Spark driver code
- Added cleanup logic to remove thread references and reset interrupt flags after statement completion
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| repl/src/main/scala/org/apache/livy/repl/Session.scala | Added statementThreads map to track interpreter threads, implemented thread interruption in cancel() method, and added cleanup in the statement execution finally block |
| repl/src/test/scala/org/apache/livy/repl/SparkSessionSpec.scala | Added test case to verify cancellation of driver code without Spark jobs, including validation that interrupted code doesn't complete and variables aren't defined |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
repl/src/test/scala/org/apache/livy/repl/SparkSessionSpec.scala
Outdated
Show resolved
Hide resolved
bd4750c to
4f99fe5
Compare
|
Addressed bot comments where applicable |
|
Hi @lmccay just wanted to gently bump if you could pls take a look thanks! |
|
There is an older PR for this issue, and one of the comments on it may be relevant here as well: #307 (review) (I haven't tested it myself) |
There was a problem hiding this comment.
Added a couple questions for clarification. Please update the description with the answers to make sure it is clear what the intent is here when done.
I'm also not very familiar with this particular code and would be more comfortable with another reviewer approving as well.
| info(s"Failed to cancel statement $statementId.") | ||
| statement.compareAndTransit(StatementState.Cancelling, StatementState.Cancelled) | ||
| } else { | ||
| Option(statementThreads.get(statementId)).foreach(_.interrupt()) |
There was a problem hiding this comment.
@ArnavBalyan - Can you please verify that this change is only intended to interrupt interruptible tasks such as sleep, object.wait, etc? As @gyogal has mentioned this will not interrupt actual long running threads.
I am also curious about the fact that this is being called within the while loop. Are we waiting for the state to be successfully set to Cancelled instead of still Cancelling? The upon failure, we set it to Cancelled after timing out therefore it will not try to be interrupted again?
What changes were proposed in this pull request?
How was this patch tested?
Closes LIVY-786