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

Extend the list of status codes retried in client #16491

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

losipiuk
Copy link
Member

@losipiuk losipiuk commented Mar 10, 2023

This commit extends list of codes on which client will retry to:

  • HTTP_BAD_GATEWAY (502)
  • HTTP_UNAVAILABLE (503)
  • HTTP_GATEWAY_TIMEOUT(504)

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Mar 10, 2023
@losipiuk losipiuk requested review from findepi and electrum March 10, 2023 11:42
@losipiuk losipiuk changed the title Retry all 50x responses in StatementClient Retry all 5xx responses in StatementClient Mar 10, 2023
@losipiuk losipiuk force-pushed the lo/retry-50x branch 2 times, most recently from 2a1bfe6 to 360bfcd Compare March 10, 2023 11:53
@losipiuk
Copy link
Member Author

updated - there was a bug there actully (== instead of !=)

@losipiuk losipiuk requested a review from findepi March 10, 2023 11:54
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

I don't think we should retry arbitrary 5xx code. Not all are appropriate, for example

  • 501 Not Implemented
  • 505 HTTP Version Not Supported

We could retry on these:

  • 502 Bad Gateway
  • 503 Service Unavailable
  • 504 Gateway Timeout

We also need to update docs/src/main/sphinx/develop/client-protocol.rst to match this change in the protocol specification.

@github-actions github-actions bot added the docs label Mar 11, 2023
@losipiuk losipiuk changed the title Retry all 5xx responses in StatementClient Extend the list of status codes retried in StatmentClient Mar 11, 2023
@losipiuk
Copy link
Member Author

Thanks for review @electrum, @findepi . Updated

@losipiuk losipiuk requested review from electrum and findepi March 11, 2023 10:19
@losipiuk losipiuk changed the title Extend the list of status codes retried in StatmentClient Extend the list of status codes retried in StatementClient Mar 11, 2023
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

I think we need to update the check in HttpTokenPoller as well, which will permanently fail the request if it is not 200 or 503.

@@ -27,8 +27,11 @@ only required on the initial ``POST`` request, and not when following the
``nextUri`` links.

If the client request returns an HTTP 503, that means the server was busy,
Copy link
Member

@electrum electrum Mar 15, 2023

Choose a reason for hiding this comment

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

Trino actually doesn't return 503. That was the intended code for gateways, etc., but in practice, we need to support 502, 503, 504, so we can just mention all three here. In other words, don't differentiate between them here, just list all three.

Copy link
Member Author

Choose a reason for hiding this comment

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

reworded docuementation.

@losipiuk losipiuk changed the title Extend the list of status codes retried in StatementClient Extend the list of status codes retried in client Mar 16, 2023
@losipiuk
Copy link
Member Author

I think we need to update the check in HttpTokenPoller

Good point. Updated that one too.

@losipiuk losipiuk requested a review from electrum March 16, 2023 08:50
This commit extends list of codes on which client will retry to:
 * HTTP_BAD_GATEWAY (502)
 * HTTP_UNAVAILABLE (503)
 * HTTP_GATEWAY_TIMEOUT(504)
@losipiuk
Copy link
Member Author

@electrum should be good to go now IMO

@losipiuk losipiuk merged commit cfe9ac9 into trinodb:master Mar 28, 2023
@github-actions github-actions bot added this to the 411 milestone Mar 28, 2023
@sajjoseph
Copy link
Contributor

Thanks for fixing this. I had this patch in our local version for a while as we had issues with trino-client code and the proxy layer before trino-server. A similar fix was suggested for trino-python-client as well.

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

Successfully merging this pull request may close these issues.

4 participants