-
Notifications
You must be signed in to change notification settings - Fork 170
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
Added retry logic for authentication with Okta #1661
Conversation
SFLoginInput loginInput = createOktaLoginInput(); | ||
Map<SFSessionProperty, Object> connectionPropertiesMap = initConnectionPropertiesMap(); | ||
SnowflakeSQLException ex = | ||
new SnowflakeSQLException(ErrorCode.AUTHENTICATOR_REQUEST_TIMEOUT, 0, true, 0); |
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 exception is thrown when timeouts are reached - what about testing 429 error code? I think that there should be a test with NETWORK_ERROR and code 429 thrown during execution
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.
Everything around checking retry related to the 429 error code happens within RestRequest.execute(). When it detects a 429 error code it eventually throws the AUTHENTICATOR_REQUEST_TIMEOUT here which is what we're testing for in this case. I've tried to see if I could set up the test to show the whole flow from a 429 error being thrown then eventually leading to getting a new Okta token, but the mocking set up gets very quickly complicated because of the level of mocks you need to set up. So what I've done instead is I've added a new test in RestRequestTest to show that it throws the AUTHENTICATOR_REQUEST_TIMEOUT when it encounters a 429 error.
Does that seem like enough test support?
…nowflake-jdbc into Okta-429-Retry-Fix
This reverts commit 75c57f2.
This reverts commit 7d7678a.
This reverts commit 5bb09bf.
This reverts commit 730fbbf.
Overview
SNOW-916942
Pre-review self checklist
master
branchmvn -P check-style validate
)mvn verify
and inspecttarget/japicmp/japicmp.html
)SNOW-XXXX:
External contributors - please answer these questions before submitting a pull request. Thanks!
Fixes https://github.com/snowflakedb/snowflake-sdks-drivers-issues-teamwork/issues/652
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.
Added logic to get a new unique token when retrying with Okta