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.

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

SNOW-878067: Retry Strategy #803

Merged
merged 24 commits into from
Nov 6, 2023
Merged

Conversation

sfc-gh-ext-simba-lf
Copy link
Collaborator

@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf commented Oct 26, 2023

Description

Regarding issue 575

The PR adds a new retry strategy for login requests with a new base time and adding jitter. It also adds the id and name to the header for login requests

New parameters for retry strategy

  • RETRY_TIMEOUT: retry timeout in seconds. defaults to 300 and can only be increased

Modified parameters

  • CONNECTION_TIMEOUT: changed default from 120 to 300

Checklist

  • Code compiles correctly
  • Code is formatted according to Coding Conventions
  • Created tests which fail without the change (if possible)
  • All tests passing (dotnet test)
  • Extended the README / documentation, if necessary
  • Provide JIRA issue id (if possible) or GitHub issue id in PR name

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #803 (4cc94f7) into master (635b214) will decrease coverage by 0.31%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
- Coverage   83.38%   83.07%   -0.31%     
==========================================
  Files          89       89              
  Lines        9075     9148      +73     
  Branches      827      835       +8     
==========================================
+ Hits         7567     7600      +33     
- Misses       1280     1323      +43     
+ Partials      228      225       -3     
Files Coverage Δ
Snowflake.Data/Core/HttpUtil.cs 94.08% <100.00%> (-2.42%) ⬇️
Snowflake.Data/Core/RestRequest.cs 89.02% <100.00%> (+0.41%) ⬆️
Snowflake.Data/Core/Session/SFSession.cs 79.41% <100.00%> (+0.10%) ⬆️
...Data/Core/Session/SFSessionHttpClientProperties.cs 94.73% <100.00%> (+2.28%) ⬆️
Snowflake.Data/Core/Session/SFSessionProperty.cs 82.08% <ø> (ø)

... and 13 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf marked this pull request as ready for review October 26, 2023 23:58
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf requested a review from a team as a code owner October 26, 2023 23:58
Snowflake.Data/Core/HttpUtil.cs Outdated Show resolved Hide resolved
Snowflake.Data/Core/HttpUtil.cs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #803 (4cc94f7) into master (635b214) will decrease coverage by 0.31%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
- Coverage   83.38%   83.07%   -0.31%     
==========================================
  Files          89       89              
  Lines        9075     9148      +73     
  Branches      827      835       +8     
==========================================
+ Hits         7567     7600      +33     
- Misses       1280     1323      +43     
+ Partials      228      225       -3     
Files Coverage Δ
Snowflake.Data/Core/HttpUtil.cs 94.08% <100.00%> (-2.42%) ⬇️
Snowflake.Data/Core/RestRequest.cs 89.02% <100.00%> (+0.41%) ⬆️
Snowflake.Data/Core/Session/SFSession.cs 79.41% <100.00%> (+0.10%) ⬆️
...Data/Core/Session/SFSessionHttpClientProperties.cs 94.73% <100.00%> (+2.28%) ⬆️
Snowflake.Data/Core/Session/SFSessionProperty.cs 82.08% <ø> (ø)

... and 13 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #803 (4cc94f7) into master (635b214) will increase coverage by 0.28%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
+ Coverage   83.38%   83.67%   +0.28%     
==========================================
  Files          89       89              
  Lines        9075     9149      +74     
  Branches      827      835       +8     
==========================================
+ Hits         7567     7655      +88     
+ Misses       1280     1268      -12     
+ Partials      228      226       -2     
Files Coverage Δ
Snowflake.Data/Core/HttpUtil.cs 96.88% <100.00%> (+0.39%) ⬆️
Snowflake.Data/Core/RestRequest.cs 89.02% <100.00%> (+0.41%) ⬆️
Snowflake.Data/Core/Session/SFSession.cs 81.00% <100.00%> (+1.69%) ⬆️
...Data/Core/Session/SFSessionHttpClientProperties.cs 94.73% <100.00%> (+2.28%) ⬆️
Snowflake.Data/Core/Session/SFSessionProperty.cs 82.08% <ø> (ø)

... and 2 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #803 (4f511f4) into master (635b214) will increase coverage by 0.16%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
+ Coverage   83.38%   83.54%   +0.16%     
==========================================
  Files          89       89              
  Lines        9075     9146      +71     
  Branches      827      835       +8     
==========================================
+ Hits         7567     7641      +74     
+ Misses       1280     1278       -2     
+ Partials      228      227       -1     
Files Coverage Δ
Snowflake.Data/Core/HttpUtil.cs 96.82% <100.00%> (+0.33%) ⬆️
Snowflake.Data/Core/RestRequest.cs 89.02% <100.00%> (+0.41%) ⬆️
Snowflake.Data/Core/Session/SFSession.cs 79.41% <100.00%> (+0.10%) ⬆️
...Data/Core/Session/SFSessionHttpClientProperties.cs 94.93% <100.00%> (+2.48%) ⬆️
Snowflake.Data/Core/Session/SFSessionProperty.cs 82.08% <ø> (ø)

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Collaborator

@sfc-gh-dstempniak sfc-gh-dstempniak left a comment

Choose a reason for hiding this comment

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

LGTM, please remember about (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))

@sfc-gh-ext-simba-lf
Copy link
Collaborator Author

LGTM, please remember about (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))

The OS check is now removed

Copy link

@ghost ghost 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-ext-simba-lf sfc-gh-ext-simba-lf merged commit 7f73791 into master Nov 6, 2023
19 of 20 checks passed
@sfc-gh-ext-simba-lf sfc-gh-ext-simba-lf deleted the SNOW-878067-Retry-Strategy branch November 6, 2023 18:06
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants