Skip to content
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

Make sure sending cancel request in StatementClient.close #133

Closed
wants to merge 1 commit into from

Conversation

takezoe
Copy link
Member

@takezoe takezoe commented Dec 6, 2024

Purpose

Make sure sending cancel request in StatementClient.close()

Overview

StatementClient.close() sends a cancel request:

def close
return unless running?
@state = :client_aborted
begin
if uri = @results.next_uri
@faraday.delete do |req|
req.url uri
end
end
rescue => e
end
nil
end
end

But it doesn't if any errors happen inside the client because state is changed to client_error in exception!():

def exception!(e)
@state = :client_error
raise e
end

This change will make sure sending a cancel request when StatementClient.close() is called unless state is finished, query_failed or client_abort.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@takezoe takezoe requested a review from a team as a code owner December 6, 2024 17:09
@takezoe takezoe force-pushed the cancel-query-in-close branch 4 times, most recently from eaf56c4 to 2c5584d Compare December 7, 2024 01:10
@takezoe takezoe changed the title Make sure sending cancel request in StatementClient.close() Make sure sending cancel request in StatementClient.close Dec 7, 2024
@takezoe takezoe force-pushed the cancel-query-in-close branch from 2c5584d to aad516e Compare December 7, 2024 02:23
@takezoe takezoe force-pushed the cancel-query-in-close branch from aad516e to 66376b8 Compare December 7, 2024 02:29
@takezoe
Copy link
Member Author

takezoe commented Dec 9, 2024

This change seems to make behavior of StatementClient inconsistent with Java's StatementClient:
https://github.com/trinodb/trino/blob/4b454b8573de6d1e4e4d32944a3e154322ab9ce3/client/trino-client/src/main/java/io/trino/client/StatementClientV1.java#L556

We will consider different ways.

@takezoe takezoe closed this Dec 9, 2024
@takezoe takezoe deleted the cancel-query-in-close branch December 9, 2024 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants