resolver/dns: Add SetMinResolutionInterval Option by HomayoonAlimohammadi · Pull Request #6962 · grpc/grpc-go

HomayoonAlimohammadi · 2024-02-04T12:24:58Z

In this merge request I tried to make SetMinResolutionInterval which is currently a constant duration variable by providing options. SetMinResolutionInterval sets the default minimum interval at which DNS re-resolutions are allowed. This helps to prevent excessive re-resolution.

RELEASE NOTES:

resolver/dns: Add SetMinResolutionInterval to set the minimum interval at which DNS re-resolutions may occur.

linux-foundation-easycla · 2024-02-04T12:25:04Z

The committers listed above are authorized under a signed CLA.

✅ login: HomayoonAlimohammadi / name: Homayoon Alimohammadi (2765325, 2398289, 06182cb)

codecov · 2024-02-04T12:27:23Z

Codecov Report

Merging #6962 (06182cb) into master (c31cec3) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6962      +/-   ##
==========================================
- Coverage   81.28%   81.19%   -0.10%     
==========================================
  Files         345      345              
  Lines       33925    33927       +2     
==========================================
- Hits        27577    27546      -31     
- Misses       5184     5208      +24     
- Partials     1164     1173       +9

Files	Coverage Δ
internal/resolver/dns/dns_resolver.go	`89.31% <100.00%> (ø)`
resolver/dns/dns_resolver.go	`66.66% <100.00%> (+16.66%)`	⬆️

... and 23 files with indirect coverage changes

HomayoonAlimohammadi · 2024-02-04T12:33:59Z

@dfawley I would really appreciate it if you would take a look at this merge request.

HomayoonAlimohammadi · 2024-02-05T11:28:21Z

I'm not able to add any labels or milestones. Would you please help me around?

HomayoonAlimohammadi · 2024-02-15T13:07:46Z

I will really appreciate it if you would take a look at this merge request: @dfawley @ginayeh

dfawley · 2024-02-15T21:01:13Z

Hi, I think a change like this is OK but we'd want it to be a global configuration knob like we are doing with DNS resolution timeouts in #6917. Please let us know if you have any questions about that approach.

github-actions · 2024-02-21T22:45:45Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

HomayoonAlimohammadi · 2024-02-25T17:40:47Z

Hi, I think a change like this is OK but we'd want it to be a global configuration knob like we are doing with DNS resolution timeouts in #6917. Please let us know if you have any questions about that approach.

Hello again, sorry for the later reply. I've changed the structure to the best of my knowledge according to the pull request you've mentioned. Please let me know if there's any problem. @dfawley

HomayoonAlimohammadi · 2024-03-11T14:00:02Z

Hi again, is there anything I can do further for this PR or is there still anything wrong with it? Thanks a lot @dfawley

HomayoonAlimohammadi · 2024-04-03T10:22:39Z

I resolved the conflicts and it's ready to go! Let me know if I need to change anything.

dfawley · 2024-04-03T22:52:38Z

Sorry for the delay.. I will review it this week for sure.

dfawley

LGTM, just a few minor comments. Thank you for the PR!

internal/resolver/dns/dns_resolver.go

dfawley · 2024-04-05T16:15:15Z

resolver/dns/dns_resolver.go

+//
+// Using this option overwrites the default [MinResolutionRate] specified
+// in the internal dns resolver package.


This is an implementation detail and should be kept out of godoc comments.

Got it! Done.

dfawley · 2024-04-05T16:16:01Z

internal/resolver/dns/dns_resolver_test.go

+	origMinResRate := dns.MinResolutionRate
+	dns.MinResolutionRate = d
+	t.Cleanup(func() { dns.MinResolutionRate = origMinResRate })


Could this call into the exported DNS library's SetMinResolution... instead, to get test coverage of the setter?

That's right, thanks for pointing it out! Done.

HomayoonAlimohammadi · 2024-04-05T16:40:47Z

I struggle to understand why the vet-proto fails. Would you please help me out?

dfawley · 2024-04-05T17:05:21Z

I struggle to understand why the vet-proto fails. Would you please help me out?

This check is optional, and the failure is caused by an upstream protobuf definition change. Please ignore it.

HomayoonAlimohammadi · 2024-04-05T17:08:01Z

I struggle to understand why the vet-proto fails. Would you please help me out?

This check is optional, and the failure is caused by an upstream protobuf definition change. Please ignore it.

Thanks a lot. I think all the comments are resolved. Let me know if I need to change anything.

dfawley · 2024-04-05T17:08:44Z

resolver/dns/dns_resolver.go

+// SetMinResolutionRate sets the default minimum rate at which DNS re-resolutions are
+// allowed. This helps to prevent excessive re-resolution.
+func SetMinResolutionRate(d time.Duration) {


I swear I wrote this comment in the last pass, but it's gone now. :) Sorry!

Please rename this to SetMinResolutionInterval (and change the global variable name to match). A "rate" is the opposite of what this is. (This actually determines the maximum rate, or the minimum interval.)

You're totally right, thanks for pointing it out!
I think I managed to change all of them. I triple checked to make sure but let me know if there's still anything wrong.

dfawley

Thanks for the PR!

arvindbr8

Looks good to me. Thanks for the PR @HomayoonAlimohammadi.

Slightly updated the PR title/description to match the latest diff.

HomayoonAlimohammadi force-pushed the add-min-dns-resolution-rate-option branch from 1a0eb21 to ffde661 Compare February 4, 2024 12:31

ginayeh requested a review from dfawley February 6, 2024 23:47

ginayeh assigned dfawley Feb 6, 2024

dfawley added the Type: Feature New features or improvements in behavior label Feb 15, 2024

dfawley added this to the 1.63 Release milestone Feb 15, 2024

dfawley assigned HomayoonAlimohammadi and unassigned dfawley Feb 15, 2024

dfawley added the Status: Requires Reporter Clarification label Feb 15, 2024

github-actions bot added stale and removed stale labels Feb 21, 2024

gnvk mentioned this pull request Feb 22, 2024

Client waits with DNS query when no channel's available #6990

Closed

HomayoonAlimohammadi force-pushed the add-min-dns-resolution-rate-option branch from ffde661 to 32b53fd Compare February 25, 2024 17:36

HomayoonAlimohammadi removed their assignment Feb 25, 2024

arvindbr8 assigned dfawley Feb 27, 2024

arvindbr8 removed the Status: Requires Reporter Clarification label Feb 27, 2024

zasweq modified the milestones: 1.63 Release, 1.64 Release Mar 20, 2024

HomayoonAlimohammadi force-pushed the add-min-dns-resolution-rate-option branch from 32b53fd to 4e95397 Compare April 2, 2024 14:51

dfawley reviewed Apr 5, 2024

View reviewed changes

dfawley assigned HomayoonAlimohammadi and unassigned dfawley Apr 5, 2024

dfawley added the Status: Requires Reporter Clarification label Apr 5, 2024

HomayoonAlimohammadi added 2 commits April 5, 2024 20:08

Add MinDNSResolutionRate Option

2765325

Fix comments

2398289

HomayoonAlimohammadi force-pushed the add-min-dns-resolution-rate-option branch from 4a02e12 to 2398289 Compare April 5, 2024 16:38

HomayoonAlimohammadi requested a review from dfawley April 5, 2024 16:39

HomayoonAlimohammadi removed their assignment Apr 5, 2024

dfawley reviewed Apr 5, 2024

View reviewed changes

Change rate to interval to correct the semantics

06182cb

dfawley approved these changes Apr 5, 2024

View reviewed changes

dfawley requested a review from arvindbr8 April 5, 2024 17:32

dfawley assigned arvindbr8 Apr 5, 2024

dfawley removed the Status: Requires Reporter Clarification label Apr 5, 2024

arvindbr8 changed the title ~~Add MinDNSResolutionRate Option~~ resolver/dns: Add SetMinResolutionInterval Option Apr 5, 2024

arvindbr8 approved these changes Apr 5, 2024

View reviewed changes

arvindbr8 merged commit 8444ae0 into grpc:master Apr 5, 2024

arvindbr8 mentioned this pull request May 9, 2024

resolver/dns: Add docstring to SetMinResolutionInterval #7217

Merged

github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2024

Conversation

HomayoonAlimohammadi commented Feb 4, 2024 • edited by dfawley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

HomayoonAlimohammadi commented Feb 4, 2024

Uh oh!

HomayoonAlimohammadi commented Feb 5, 2024

Uh oh!

HomayoonAlimohammadi commented Feb 15, 2024

Uh oh!

dfawley commented Feb 15, 2024

Uh oh!

github-actions bot commented Feb 21, 2024

Uh oh!

HomayoonAlimohammadi commented Feb 25, 2024

Uh oh!

HomayoonAlimohammadi commented Mar 11, 2024

Uh oh!

HomayoonAlimohammadi commented Apr 3, 2024

Uh oh!

dfawley commented Apr 3, 2024

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dfawley Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

dfawley Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi commented Apr 5, 2024

Uh oh!

dfawley commented Apr 5, 2024

Uh oh!

HomayoonAlimohammadi commented Apr 5, 2024

Uh oh!

dfawley Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

HomayoonAlimohammadi Apr 5, 2024

Choose a reason for hiding this comment

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

arvindbr8 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HomayoonAlimohammadi commented Feb 4, 2024 •

edited by dfawley

Loading

linux-foundation-easycla bot commented Feb 4, 2024 •

edited

Loading

codecov bot commented Feb 4, 2024 •

edited

Loading