-
Notifications
You must be signed in to change notification settings - Fork 33
Fix: try cancelling statement in all cases before throing an error #1202
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -254,6 +254,8 @@ public DatabricksResultSet executeStatement( | |||||||
| try { | ||||||||
| Thread.sleep(connectionContext.getAsyncExecPollInterval()); | ||||||||
| } catch (InterruptedException e) { | ||||||||
| // Cancel the statement on the server before throwing - it may still be running | ||||||||
| tryCancelStatement(typedStatementId, "thread interruption"); | ||||||||
| String timeoutErrorMessage = | ||||||||
| String.format( | ||||||||
| "Thread interrupted due to statement timeout. StatementID %s", statementId); | ||||||||
|
Comment on lines
254
to
261
|
||||||||
|
|
@@ -268,6 +270,8 @@ public DatabricksResultSet executeStatement( | |||||||
| req.withHeaders(getHeaders("getStatement")); | ||||||||
| response = wrapGetStatementResponse(apiClient.execute(req, GetStatementResponse.class)); | ||||||||
| } catch (IOException e) { | ||||||||
| // Cancel the statement on the server before throwing - it may still be running | ||||||||
| tryCancelStatement(typedStatementId, "polling error"); | ||||||||
| String errorMessage = "Error while processing the get statement response"; | ||||||||
| LOGGER.error(errorMessage, e); | ||||||||
| throw new DatabricksSQLException( | ||||||||
|
|
@@ -428,6 +432,33 @@ public void cancelStatement(StatementId typedStatementId) throws DatabricksSQLEx | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Attempts to cancel a statement, logging any errors but not throwing. | ||||||||
| * | ||||||||
| * <p>This method is used during error handling to ensure statements are cancelled on the server | ||||||||
| * when the client encounters transient errors during polling. Since the statement may still be | ||||||||
| * running on the server, we attempt to cancel it to avoid resource leaks. | ||||||||
| * | ||||||||
| * @param statementId The statement ID to cancel | ||||||||
| * @param reason The reason for cancellation (for logging) | ||||||||
| */ | ||||||||
| private void tryCancelStatement(StatementId statementId, String reason) { | ||||||||
| try { | ||||||||
| LOGGER.debug( | ||||||||
| "Attempting to cancel statement {} due to {}", | ||||||||
| statementId.toSQLExecStatementId(), | ||||||||
| reason); | ||||||||
| cancelStatement(statementId); | ||||||||
| LOGGER.debug("Successfully cancelled statement {} due to {}", statementId, reason); | ||||||||
| } catch (Exception e) { | ||||||||
| LOGGER.warn( | ||||||||
| "Failed to cancel statement {} after {}: {}", | ||||||||
| statementId.toSQLExecStatementId(), | ||||||||
| reason, | ||||||||
| e.getMessage()); | ||||||||
|
||||||||
| e.getMessage()); | |
| e.getMessage(), | |
| e); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -169,6 +169,36 @@ TCancelOperationResp cancelOperation(TCancelOperationReq req) throws DatabricksH | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * Attempts to cancel an operation, logging any errors but not throwing. | ||||||||||||||||||
| * | ||||||||||||||||||
| * <p>This method is used during error handling to ensure operations are cancelled on the server | ||||||||||||||||||
| * when the client encounters transient errors during polling. Since the operation may still be | ||||||||||||||||||
| * running on the server, we attempt to cancel it to avoid resource leaks. | ||||||||||||||||||
| * | ||||||||||||||||||
| * @param operationHandle The operation handle to cancel | ||||||||||||||||||
| * @param statementId The statement ID (for logging) | ||||||||||||||||||
| * @param reason The reason for cancellation (for logging) | ||||||||||||||||||
| */ | ||||||||||||||||||
| private void tryCancelOperation( | ||||||||||||||||||
| TOperationHandle operationHandle, StatementId statementId, String reason) { | ||||||||||||||||||
| try { | ||||||||||||||||||
| LOGGER.debug( | ||||||||||||||||||
| "Attempting to cancel operation for statement {} due to {}", | ||||||||||||||||||
| statementId.toSQLExecStatementId(), | ||||||||||||||||||
| reason); | ||||||||||||||||||
| cancelOperation(new TCancelOperationReq().setOperationHandle(operationHandle)); | ||||||||||||||||||
| LOGGER.debug( | ||||||||||||||||||
| "Successfully cancelled operation for statement {} due to {}", statementId, reason); | ||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||
| LOGGER.warn( | ||||||||||||||||||
| "Failed to cancel operation for statement {} after {}: {}", | ||||||||||||||||||
| statementId.toSQLExecStatementId(), | ||||||||||||||||||
| reason, | ||||||||||||||||||
| e.getMessage()); | ||||||||||||||||||
|
Comment on lines
+195
to
+198
|
||||||||||||||||||
| "Failed to cancel operation for statement {} after {}: {}", | |
| statementId.toSQLExecStatementId(), | |
| reason, | |
| e.getMessage()); | |
| "Failed to cancel operation for statement {} after {}", | |
| statementId.toSQLExecStatementId(), | |
| reason, | |
| e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
restore interrupt flag -> can we add this to sdkClient handling as well?
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -953,4 +953,38 @@ private TFetchResultsReq getFetchResultsRequest(boolean includeMetadata) | |||||||||
| } | ||||||||||
| return request; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| @Test | ||||||||||
| public void testPollingErrorCancelsOperation() throws Exception { | ||||||||||
| // Test that when GetOperationStatus fails with TException, the operation is cancelled | ||||||||||
| TExecuteStatementReq request = new TExecuteStatementReq(); | ||||||||||
|
|
||||||||||
| // Response with RUNNING state - requires polling | ||||||||||
| TExecuteStatementResp tExecuteStatementResp = | ||||||||||
| new TExecuteStatementResp() | ||||||||||
| .setOperationHandle(tOperationHandle) | ||||||||||
| .setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS)); | ||||||||||
|
|
||||||||||
| Statement statement = mock(Statement.class); | ||||||||||
| when(parentStatement.getStatement()).thenReturn(statement); | ||||||||||
| when(statement.getQueryTimeout()).thenReturn(0); | ||||||||||
| when(thriftClient.ExecuteStatement(request)).thenReturn(tExecuteStatementResp); | ||||||||||
|
|
||||||||||
| // First GetOperationStatus returns RUNNING, second throws TException | ||||||||||
| when(thriftClient.GetOperationStatus(operationStatusReq)) | ||||||||||
| .thenReturn(operationStatusRunningResp) | ||||||||||
| .thenThrow(new TException("Network error during polling")); | ||||||||||
|
|
||||||||||
| // Mock CancelOperation to succeed | ||||||||||
| when(thriftClient.CancelOperation(any(TCancelOperationReq.class))) | ||||||||||
| .thenReturn(new TCancelOperationResp()); | ||||||||||
|
||||||||||
| .thenReturn(new TCancelOperationResp()); | |
| .thenReturn( | |
| new TCancelOperationResp() | |
| .setStatus(new TStatus().setStatusCode(TStatusCode.SUCCESS_STATUS))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have cancel being fired from checkTimeout above also. We can keep a check that if cancel was already attempted, don't do cancel again.