-
Notifications
You must be signed in to change notification settings - Fork 6
(feat) FIR-42058 add the support for statement_timeout #563
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: master
Are you sure you want to change the base?
(feat) FIR-42058 add the support for statement_timeout #563
Conversation
|
bogdantruta-firebolt
left a comment
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.
lgtm
| statement.setQueryTimeout(1); | ||
| ResultSet resultSet = statement.executeQuery("SELECT * FROM GENERATE_SERIES(1, 10000000);"); | ||
| boolean errorFound = false; | ||
| while (resultSet.next()) { | ||
| String object = resultSet.getString(1); | ||
| if (object.contains("Query was canceled with reason 'Query timeout expired (1 ms)")) { | ||
| errorFound = true; | ||
| } | ||
| } |
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.
I think we're meant to raise a Timeout exception here
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.
This one is a bit tricky. The spec mentions that a timeout exception should be thrown.
But I think that would be the case if the server did not start sending any response (for example, compute a complex join query). In this case, the server starts sending some response.
That's why I was asking in the Jira what is the expected behavior since the driver did not throw any exception even before with max_execution_time.
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.
I think max_execution_time was never actually working on 2.0, it was ported from 1.0. Actually, just checked and it doesn't do anything on 1.0 right now either, so we shouldn't rely on the past behavior here.
Server side is separate and, as mentioned in the ticket, doesn't seem to be working correctly. However, we're concerned with the client-side behavior here and the expectation is that we throw an exception. If we can't do that with the server-side parameter then we would have to implement timeout on the client side and abort the connection when timeout is reached.



When setting a timeout for a query we JDBC driver was sending the query param to the server as: max_execution_time.
This was changed to statement_timeout so when a timeout is set on the statement the driver will add a query parameter as statement_timeout.