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.

Sign up for GitHub

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

Jump to bottom

SNOW-577665: Wrong retry 403 logic #916

Merged
merged 4 commits into from
Apr 21, 2022
Merged

SNOW-577665: Wrong retry 403 logic #916

merged 4 commits into from
Apr 21, 2022

Conversation

sfc-gh-wshangguan
Copy link
Contributor

Overview

SNOW-577665

Bug fix. The bug was introduced in 6594bc4

The isNonRetryableHttpCode function returns wrong value for 403 http code. Please look at RestRequestTest.java for correct behavior on 403.

External contributors - please answer these questions before submitting a pull request. Thanks!

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR adressing? Make sure that there is an accompanying issue to your PR.

    Fixes #NNNN

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am modyfying authorization mechanisms
    • I am adding new credentials
    • I am modyfying OCSP code
    • I am adding a new dependency

  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

Pre-review checklist

  • This change has passed precommit
  • I have reviewed code coverage report for my PR in (Sonarqube)

sfc-gh-wshangguan
sfc-gh-wshangguan commented Apr 19, 2022
View reviewed changes
src/main/java/net/snowflake/client/jdbc/RestRequest.java
return response != null
&& (response.getStatusLine().getStatusCode() < 500
|| // service unavailable
response.getStatusLine().getStatusCode() >= 600)
&& // gateway timeout
response.getStatusLine().getStatusCode() != 408
&& // request timeout
(retryHTTP403 || response.getStatusLine().getStatusCode() != 403);
(!retryHTTP403 || response.getStatusLine().getStatusCode() != 403);
Copy link
Contributor Author

@sfc-gh-wshangguan sfc-gh-wshangguan Apr 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is the fix. Reviewers please carefully review whether this change is correct since it's error prone. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense!

sfc-gh-hchaturvedi
sfc-gh-hchaturvedi previously approved these changes Apr 19, 2022
View reviewed changes
Copy link
Collaborator

@sfc-gh-hchaturvedi sfc-gh-hchaturvedi left a comment

Choose a reason for hiding this comment

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

LGTM

@sfc-gh-wshangguan sfc-gh-wshangguan force-pushed the wshangguan-SNOW-577665-JDBC-wrong-retry-403-logic branch from 1c1cabf to 749c67e Compare April 19, 2022 21:41
@sfc-gh-hchaturvedi
Copy link
Collaborator

@sfc-gh-wshangguan do we know why the tests are failing and that too only FIPS tests?

@sonarqubemergegate
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sfc-gh-wshangguan
Copy link
Contributor Author

@sfc-gh-wshangguan do we know why the tests are failing and that too only FIPS tests?

Not really, but it started to fail before this change.

sfc-gh-wshangguan
sfc-gh-wshangguan commented Apr 20, 2022
View reviewed changes
FIPS/src/test/java/net/snowflake/client/jdbc/ConnectionFipsIT.java Outdated
Comment on lines 105 to 106
// TODO: temporarily disable the function to merge an urgent pr
// @BeforeClass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mknister , since the tag is @BeforeClass, only ignoring tests is not powerful enough. I have to temporarily comment out this tag to get the test pass. We can add this back after merging the code. You think this is acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to have you make more changes, but could you revert all changes to this file and then rebase your other commits on top of the current master branch? JDBC4-master tests are now fixed (in a different way than adding @ignore)

sfc-gh-wshangguan
sfc-gh-wshangguan commented Apr 20, 2022
View reviewed changes
FIPS/src/test/java/net/snowflake/client/jdbc/ConnectionFipsIT.java Outdated
Comment on lines 166 to 167
// TODO: temporarily disable the function to merge an urgent pr
// @AfterClass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here.

sfc-gh-mknister
sfc-gh-mknister reviewed Apr 20, 2022
View reviewed changes
Copy link
Contributor

@sfc-gh-mknister sfc-gh-mknister left a comment

Choose a reason for hiding this comment

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

Actual changes look good! Sorry for extra work- if you could take away ConnectionFipsIT changes + put your changes on top of current master, that would be great.

@sfc-gh-wshangguan sfc-gh-wshangguan force-pushed the wshangguan-SNOW-577665-JDBC-wrong-retry-403-logic branch from b908a3d to 42ff717 Compare April 20, 2022 19:06
sfc-gh-mknister
sfc-gh-mknister approved these changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@sfc-gh-mknister sfc-gh-mknister left a comment

Choose a reason for hiding this comment

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

LGTM!

@sfc-gh-wshangguan sfc-gh-wshangguan merged commit cd3b331 into master Apr 21, 2022
@sfc-gh-wshangguan sfc-gh-wshangguan deleted the wshangguan-SNOW-577665-JDBC-wrong-retry-403-logic branch April 21, 2022 03:36
oodama pushed a commit to oodama/snowflake-jdbc that referenced this pull request May 24, 2022
@sfc-gh-wshangguan
SNOW-577665: Wrong retry 403 logic (snowflakedb#916)
d78947e 
Fixed the bug for isNonRetryableHttpCode(403, true/false)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-mknister sfc-gh-mknister sfc-gh-mknister approved these changes

@sfc-gh-hchaturvedi sfc-gh-hchaturvedi Awaiting requested review from sfc-gh-hchaturvedi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sfc-gh-wshangguan @sfc-gh-hchaturvedi @sfc-gh-mknister