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

fix: Retry POST against 502, 503 and 504 responses #134

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kar0t
Copy link
Contributor

@kar0t kar0t commented Dec 10, 2024

Purpose

Retry POST against 50X responses.

Here's what I'd like to see reviewed

  1. Please confirm that it is okay to handle Timeout within the POST function.
  2. I am not familiar with the Ruby language, so there may be some errors. I have written it to follow the same logic as the existing faraday_get_with_retry as much as possible.

Overview

According to the trino client protocol, it should also retry for POST.

https://trino.io/docs/current/develop/client-protocol.html#overview-of-query-processing
If the client request returns an HTTP 502, 503, or 504, that means there was an intermittent problem processing request and the client should try again in 50-100 ms. Trino does not generate those codes by itself, but those can be generated by load balancers in front of Trino.

Checklist

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

@kar0t kar0t requested a review from a team as a code owner December 10, 2024 14:19
@github-actions github-actions bot added the bug label Dec 10, 2024
@kar0t kar0t force-pushed the 1ambda-patch-trino-retry-50x-post branch from 7e6ea27 to 67484cc Compare December 10, 2024 14:47
@kar0t kar0t force-pushed the 1ambda-patch-trino-retry-50x-post branch from 67484cc to a2dc9e5 Compare December 10, 2024 14:47
@yuokada yuokada requested a review from exoego December 12, 2024 00:33
Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test.

it "raises TrinoQueryTimeoutError if timeout during execution" do
stub_request(:post, "localhost/v1/statement").
with(body: query, headers: headers).
to_return(body: planning_response.to_json)
client = StatementClient.new(faraday, query, options.merge(query_timeout: 1))
stub_request(:get, "localhost/v1/next_uri").
with(headers: headers).
to_return(body: early_running_response.to_json)
client.advance
sleep 1
stub_request(:get, "localhost/v1/next_uri").
with(headers: headers).
to_return(body: late_running_response.to_json)
expect do
client.advance
end.to raise_error(Trino::Client::TrinoQueryTimeoutError, "Query queryid timed out")
end
it "doesn't raise errors if query is done" do
stub_request(:post, "localhost/v1/statement").
with(body: query, headers: headers).
to_return(body: planning_response.to_json)
client = StatementClient.new(faraday, query, options.merge(query_timeout: 1))
stub_request(:get, "localhost/v1/next_uri").
with(headers: headers).
to_return(body: early_running_response.to_json)
client.advance
stub_request(:get, "localhost/v1/next_uri").
with(headers: headers).
to_return(body: done_response.to_json)
client.advance # set finished
sleep 1
client.advance # set finished
end
is an example

@kar0t
Copy link
Contributor Author

kar0t commented Dec 16, 2024

I am writing a test for the modified post_query_request!.
Will update this PR soon. Thanks for the reivew @exoego!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants