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

refactor:rename clientside dialoption field 'maxAttempts' #7391

Closed
wants to merge 3 commits into from

Conversation

No-SilverBullet
Copy link

@No-SilverBullet No-SilverBullet commented Jul 4, 2024

RELEASE NOTES:

  • Renamed the variable maxCallAttempts to maxRetryAttempts to improve clarity and better represent its purpose.

Recently, I noticed a bug with the RPC client's setting of the maximum number of reconnections. I was very surprised to find that it has been solved in this PR client: implement maxAttempts for retryPolicy . However, I think the meaning of this variable 'maxCallAttempts' are not clear enough, so I submitted this PR to rename this variable.

Copy link

linux-foundation-easycla bot commented Jul 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.40%. Comparing base (bdd707e) to head (72b2cf2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7391      +/-   ##
==========================================
- Coverage   81.42%   81.40%   -0.02%     
==========================================
  Files         348      348              
  Lines       26744    26744              
==========================================
- Hits        21775    21772       -3     
- Misses       3779     3783       +4     
+ Partials     1190     1189       -1     
Files Coverage Δ
clientconn.go 93.14% <100.00%> (ø)
dialoptions.go 89.95% <100.00%> (ø)
resolver_wrapper.go 85.10% <100.00%> (ø)
service_config.go 82.35% <100.00%> (ø)

... and 14 files with indirect coverage changes

@No-SilverBullet No-SilverBullet changed the title Refactor:rename clientside dialoption field 'maxAttempts' refactor:rename clientside dialoption field 'maxAttempts' Jul 4, 2024
@purnesh42H purnesh42H assigned purnesh42H and arjan-bal and unassigned purnesh42H Jul 5, 2024
@arjan-bal
Copy link
Contributor

arjan-bal commented Jul 5, 2024

Hi @No-SilverBullet, I agree that the name could be better. The API that users interact with is the WithMaxCallAttempt dialoption.

grpc-go/dialoptions.go

Lines 752 to 759 in bdd707e

func WithMaxCallAttempts(n int) DialOption {
return newFuncDialOption(func(o *dialOptions) {
if n < 2 {
n = defaultMaxCallAttempts
}
o.maxCallAttempts = n
})
}

If we were to change the name of WithMaxCallAttempt, it would be a breaking change. To handle it gracefully, we would need to create a dial option with the new name, deprecate the old one and then completely remove the old one in a future release. This doesn't seem to be worth the effort so I'm closing this PR.

Since the external method is named as such, IMO keeping the private variable names consistent with it seems to desired bahaviour.

@arjan-bal arjan-bal closed this Jul 5, 2024
@No-SilverBullet
Copy link
Author

Hi @No-SilverBullet, I agree that the name could be better. The API that users interact with is the WithMaxCallAttempt dialoption.

grpc-go/dialoptions.go

Lines 752 to 759 in bdd707e

func WithMaxCallAttempts(n int) DialOption {
return newFuncDialOption(func(o *dialOptions) {
if n < 2 {
n = defaultMaxCallAttempts
}
o.maxCallAttempts = n
})
}

If we were to change the name of WithMaxCallAttempt, it would be a breaking change. To handle it gracefully, we would need to create a dial option with the new name, deprecate the old one and then completely remove the old one in a future release. This doesn't seem to be worth the effort so I'm closing this PR.

Since the external method is named as such, IMO keeping the private variable names consistent with it seems to desired bahaviour.

Got it! Thanks for reply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants