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

Integrating max retry attempts with client and changing the default retry attempts to 0 #2131

Merged
merged 25 commits into from
Jul 18, 2024

Conversation

Tulsishah
Copy link
Collaborator

@Tulsishah Tulsishah commented Jul 8, 2024

Description

  • Integrating max-retry-attempts in storage client to stop infinite retries on retry able errors.
  • Changing default value to 0 to be backward compatible. setting max retry attempts to 0 implies infinite retries.

Link to the issue in case of a bug fix.

#1476

Testing details

  1. Manual -
    Tested flag with storage test bench server.
    1. Cmd line (max-retry-attempts = 2) and config file flag not set.
      two retries occurred.
    2. Cmd line (max-retry-attempts = 2 )and config file (max-retry-attempts = 5)
      two retries occurred.
    3. Cmd line (max-retry-attempts not set) and config file (max-retry-attempts = 5)
      five retries occurred.
    4. Cmd line (max-retry-attempts not set) and config file (max-retry-attempts not set) = default 0 which means infinite
      Infinite retries occurred.
  2. Unit tests - NA
  3. Integration tests - NA

@Tulsishah Tulsishah added the execute-integration-tests Run only integration tests label Jul 8, 2024
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.00%. Comparing base (dd5bc06) to head (48cf255).
Report is 34 commits behind head on master.

Files Patch % Lines
cfg/config.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2131      +/-   ##
==========================================
+ Coverage   71.73%   72.00%   +0.26%     
==========================================
  Files         101      101              
  Lines       11117    11168      +51     
==========================================
+ Hits         7975     8041      +66     
+ Misses       2813     2797      -16     
- Partials      329      330       +1     
Flag Coverage Δ
unittests 72.00% <85.71%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tulsishah Tulsishah force-pushed the integrate_max_retry_attempt branch from 7c684ec to 7b4b0e8 Compare July 9, 2024 05:48
@Tulsishah Tulsishah marked this pull request as ready for review July 9, 2024 05:48
@Tulsishah Tulsishah requested review from kislaykishore and a team as code owners July 9, 2024 05:48
@Tulsishah Tulsishah changed the title Integrating max retry attempts Integrating max retry attempts with client Jul 9, 2024
@Tulsishah Tulsishah force-pushed the integrate_max_retry_attempt branch 2 times, most recently from e2a912d to 906b53a Compare July 9, 2024 09:11
@Tulsishah Tulsishah added execute-integration-tests Run only integration tests and removed execute-integration-tests Run only integration tests labels Jul 9, 2024
@Tulsishah Tulsishah force-pushed the integrate_max_retry_attempt branch from 7a546f7 to 883f67f Compare July 10, 2024 06:27
Copy link
Collaborator

@kislaykishore kislaykishore left a comment

Choose a reason for hiding this comment

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

The title appears misleading; should it be something like: "Changing the default retry attempts to 0". Also, in description we can add that "setting max retry attempts to 0 implies infinite retries".

internal/storage/storage_handle_test.go Outdated Show resolved Hide resolved
@Tulsishah Tulsishah changed the title Integrating max retry attempts with client Integrating max retry attempts with client and changing the default retry attempts to 0 Jul 10, 2024
@Tulsishah
Copy link
Collaborator Author

The title appears misleading; should it be something like: "Changing the default retry attempts to 0". Also, in description we can add that "setting max retry attempts to 0 implies infinite retries".

Done

kislaykishore
kislaykishore previously approved these changes Jul 11, 2024
internal/storage/storage_handle_test.go Outdated Show resolved Hide resolved
Co-authored-by: Kislay Kishore <kislayk@google.com>
@Tulsishah Tulsishah merged commit 24a1871 into master Jul 18, 2024
13 checks passed
@Tulsishah Tulsishah deleted the integrate_max_retry_attempt branch July 19, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execute-integration-tests Run only integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants