[Improvement-17695][ProcedureTask] Support cancel procedure task #17696
Conversation
…heduler into Improvement-17695
|
There was a problem hiding this comment.
Pull request overview
This pull request adds cancellation support for procedure tasks by implementing the cancel() method to call Statement.cancel() on the active database statement. This allows procedure tasks to be interrupted during execution, similar to other task types like SQL tasks.
Key Changes
- Implemented
cancel()method inProcedureTaskto support task cancellation viaStatement.cancel() - Added thread-safe volatile
sessionStatementfield to track the active statement for cancellation - Enhanced error handling to distinguish between cancelled and failed tasks
- Added comprehensive unit tests covering various cancellation scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| ProcedureTask.java | Implemented cancel() method with statement cancellation, added volatile sessionStatement field, and enhanced exception handling to check for EXIT_CODE_KILL |
| ProcedureTaskTest.java | Added comprehensive unit tests for constructor validation, successful execution, error handling, cancellation scenarios, timeout configuration, and SQL formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| stmt.cancel(); | ||
| setExitStatusCode(TaskConstants.EXIT_CODE_KILL); |
There was a problem hiding this comment.
The exit status code should be set to EXIT_CODE_KILL before attempting to cancel the statement. If stmt.cancel() throws an exception, the exit status will not be set, which could lead to incorrect task status reporting. Consider moving line 144 before line 143.
| stmt.cancel(); | |
| setExitStatusCode(TaskConstants.EXIT_CODE_KILL); | |
| setExitStatusCode(TaskConstants.EXIT_CODE_KILL); | |
| stmt.cancel(); |
There was a problem hiding this comment.
The exit status code should be set to EXIT_CODE_KILL before attempting to cancel the statement. If stmt.cancel() throws an exception, the exit status will not be set, which could lead to incorrect task status reporting. Consider moving line 144 before line 143.
good
| Integer exitCode = getPrivateField(procedureTask, "exitStatusCode"); | ||
| assertEquals(exitCode, -1); |
There was a problem hiding this comment.
The test expects exitStatusCode to be -1 when cancel() throws an exception, but this contradicts the bug found in the implementation. According to line 144 of ProcedureTask.java, if stmt.cancel() throws an exception, setExitStatusCode() is never called, so the exit code remains at its previous value. The test assumption that it should be -1 is incorrect unless that's the default value. Consider verifying what the actual initial/default exit status code is, or update this test to reflect the actual behavior after fixing the implementation bug.
| Integer exitCode = getPrivateField(procedureTask, "exitStatusCode"); | |
| assertEquals(exitCode, -1); | |
| Integer exitCodeBefore = getPrivateField(procedureTask, "exitStatusCode"); | |
| Integer exitCodeAfter = getPrivateField(procedureTask, "exitStatusCode"); | |
| assertEquals(exitCodeAfter, exitCodeBefore, "exitStatusCode should remain unchanged when cancel() throws"); |
| @Override | ||
| public void cancel() throws TaskException { | ||
|
|
||
| public void cancel() { |
There was a problem hiding this comment.
The cancel() method signature was changed to remove the throws TaskException declaration, which is inconsistent with the parent class AbstractTask that declares public abstract void cancel() throws TaskException;. Even though the implementation throws TaskException on line 148, the signature should maintain the throws declaration for consistency with the parent class and other task implementations.
| public void cancel() { | |
| public void cancel() throws TaskException { |
There was a problem hiding this comment.
The cancel() method signature was changed to remove the
throws TaskExceptiondeclaration, which is inconsistent with the parent class AbstractTask that declarespublic abstract void cancel() throws TaskException;. Even though the implementation throws TaskException on line 148, the signature should maintain the throws declaration for consistency with the parent class and other task implementations.
ok,good
| private ResourceParametersHelper mockResourceHelper; | ||
|
|
||
| @Mock | ||
| private ProcedureParameters procedureParams = new ProcedureParameters(); |
There was a problem hiding this comment.
The @mock annotation should not be used with initialization. On line 73, procedureParams is annotated with @mock but also initialized with = new ProcedureParameters(). This is contradictory - either use @mock alone or remove the annotation and just instantiate. The initialization will be overwritten by Mockito anyway.
| private ProcedureParameters procedureParams = new ProcedureParameters(); | |
| private ProcedureParameters procedureParams; |
| log.debug("this procedure task was canceled"); | ||
| } catch (SQLException ex) { | ||
| log.warn("Failed to cancel stored procedure (driver/DB may not support it)", ex); | ||
| throw new TaskException("Cancel http task failed", ex); |
There was a problem hiding this comment.
The error message refers to "http task" but this is a procedure task. The message should be "Cancel procedure task failed" instead of "Cancel http task failed".
| throw new TaskException("Cancel http task failed", ex); | |
| throw new TaskException("Cancel procedure task failed", ex); |
There was a problem hiding this comment.
The error message refers to "http task" but this is a procedure task. The message should be "Cancel procedure task failed" instead of "Cancel http task failed".
nice
Purpose of the pull request
close #17695
Brief change log
call statement.cancel method to cancel the procedure task
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
Pull Request Notice
Pull Request Notice
If your pull request contains incompatible change, you should also add it to
docs/docs/en/guide/upgrade/incompatible.md