SNOW-1524152: Implement setQueryTimeout for async queries #1958
Conversation
| @Test | ||
| public void testSetQueryTimeoutForAsnycQuery() throws SQLException { | ||
| try (Statement statement = connection.createStatement()) { | ||
| statement.unwrap(SnowflakeStatement.class).setParameter("MULTI_STATEMENT_COUNT", 0); |
There was a problem hiding this comment.
why do we need multistatements? we are sendinf single query
|
|
||
| QueryStatus queryStatus = QueryStatus.RUNNING; | ||
| while (queryStatus == QueryStatus.RUNNING | ||
| || queryStatus == QueryStatus.RESUMING_WAREHOUSE) { |
There was a problem hiding this comment.
resuming warehouse it's not an error we should check - if we want to avoid it let's run a normal query without timeout before to ensure that Warehouse is ready
| statement.unwrap(SnowflakeStatement.class).executeAsyncQuery(sql)) { | ||
|
|
||
| QueryStatus queryStatus = QueryStatus.RUNNING; | ||
| while (queryStatus == QueryStatus.RUNNING |
There was a problem hiding this comment.
let's use awaitility for waiting
| } catch (InterruptedException e) { | ||
| fail(e.getMessage()); | ||
| } | ||
| queryStatus = resultSet.unwrap(SnowflakeResultSet.class).getStatus(); |
There was a problem hiding this comment.
we should use getStatusV2
| queryStatus = resultSet.unwrap(SnowflakeResultSet.class).getStatus(); | ||
| } | ||
|
|
||
| if (queryStatus == QueryStatus.FAILED_WITH_ERROR) { |
There was a problem hiding this comment.
let's not have the if - it should always fail, checkout my fix in #1974
| statement.unwrap(SnowflakeStatement.class).setParameter("MULTI_STATEMENT_COUNT", 0); | ||
| statement.setQueryTimeout(3); | ||
|
|
||
| String sql = "SELECT * FROM SNOWFLAKE_SAMPLE_DATA.TPCDS_SF100TCL.CUSTOMER;"; |
There was a problem hiding this comment.
can we use gnerated data instead of predefined?
| public void testSetQueryTimeoutForAsnycQuery() throws SQLException { | ||
| try (Statement statement = connection.createStatement()) { | ||
| statement.unwrap(SnowflakeStatement.class).setParameter("MULTI_STATEMENT_COUNT", 0); | ||
| statement.setQueryTimeout(3); |
There was a problem hiding this comment.
the connection is reused now in this test so when we set the timeout all the tests in this class will have the timeout - we should have local connection or reset query timeout in finally
| queryTimeout = (Integer) propertyValue; | ||
| // Set server parameter for supporting query timeout on async queries | ||
| statementParametersMap.put("STATEMENT_TIMEOUT_IN_SECONDS", (Integer) propertyValue); |
There was a problem hiding this comment.
I am also not sure if we want to mix the behaviour here for sync and async queries - do our users receive exactly the same error as before when driver internall was timing out?
To play safe I would suggest creating new method in SnowflakeStatement with explicit name setAsyncQueryTimeout
in the existing setQueryTimeout method I would guard the STATEMENT_TIMEOUT_IN_SECONDS with some new driver parameter with default to false e.g. SUPPORT_IMPLICIT_ASYNC_QUERY_TIMEOUT
@sfc-gh-abenzaoui WDYT?
sfc-gh-dprzybysz
changed the title
Overview
SNOW-1524152
Pre-review self checklist
masterbranchmvn -P check-style validate)mvn verifyand inspecttarget/japicmp/japicmp.html)SNOW-XXXX:External contributors - please answer these questions before submitting a pull request. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Issue: #NNNN
Fill out the following pre-review checklist:
@SnowflakeJdbcInternalApi(note that public/protected methods/fields in classes marked with this annotation are already internal)Please describe how your code solves the related issue.
Please write a short description of how your code change solves the related issue.