Snow 1853185 jdbc driver v3.16 native okta http retry storm #2064
Conversation
… into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta-HTTP-Retry-Storm
…ative-Okta-HTTP-Retry-Storm' into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta-HTTP-Retry-Storm
…-HTTP-Retry-Storm
| @@ -503,6 +527,9 @@ public static CloseableHttpResponse execute( | |||
| requestIdStr, | |||
| requestInfoScrubbed, | |||
| backoffInMilli); | |||
| // TODO: shouldn't we sleep here for backoffInMilli - elapsedMilliForLastCall | |||
| // ? | |||
| // Thread.sleep(backoffInMilli - elapsedMilliForLastCall); | |||
| Thread.sleep(backoffInMilli); | |||
There was a problem hiding this comment.
Shouldn't we sleep here for backoffInMilli - elapsedMilliForLastCall?
There was a problem hiding this comment.
I don't think so. BackoffInMillis is calculated to be used as a time to sleep. Do you agree?
There was a problem hiding this comment.
I would assume we may want to define 'backoff' as the time we want to have elapsed before sending the next request. In such way we would like to subtract the amount of time already spent for the processing, from the backoff-time - so the total waited time is correct.
It also seems to have been the original idea, taking for a word this comment:
However it is more like a clue for the future changes - not to be introduced in this PR.
…-HTTP-Retry-Storm
This reverts commit 618e9b3.
| try { | ||
| String newOneTimeToken = oneTimeTokenSupplier.apply(retryContext); | ||
| prepareFederatedFlowStep4Request(retrieveSamlRequest, ssoUrl, newOneTimeToken); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Should we end with a Runtime exception here? I see that there were MalformedURLException | URISyntaxException so we can continue with SnowflakeSQLException, true? Could we use here handleFederatedFlowError?
There was a problem hiding this comment.
That's right - thanks - added
| @@ -503,6 +527,9 @@ public static CloseableHttpResponse execute( | |||
| requestIdStr, | |||
| requestInfoScrubbed, | |||
| backoffInMilli); | |||
| // TODO: shouldn't we sleep here for backoffInMilli - elapsedMilliForLastCall | |||
| // ? | |||
| // Thread.sleep(backoffInMilli - elapsedMilliForLastCall); | |||
| Thread.sleep(backoffInMilli); | |||
There was a problem hiding this comment.
I don't think so. BackoffInMillis is calculated to be used as a time to sleep. Do you agree?
| @@ -630,4 +643,28 @@ public void testOktaWithInvalidHostName() throws Throwable { | |||
| assertEquals((int) ErrorCode.IDP_INCORRECT_DESTINATION.getMessageCode(), ex.getErrorCode()); | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testOktaAuthRequestsAreRetriedUsingLoginRetryStrategy() { | |||
There was a problem hiding this comment.
Should we create the opposite test? I mean, where we confirme, then other login requests don't use RetryStrategy?
This reverts commit f8aeb21.
…st to be recognized as login
Overview
SNOW-1853185
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.