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

Fix/726 738 dns cache refresh impr #1637

Closed
wants to merge 4 commits into from

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Sep 22, 2020

Things that need to be done:

  • Dialer needs a logger
  • remove no longer needed Resolver type (this would've broken a lot more code so I deprioritized it), or rewrite the new code to be called Resolver
  • Tests Tests Tests
  • make the dns cache shared between VUs again

@mstoykov mstoykov requested review from imiric and na-- September 22, 2020 10:48
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

I ran a few tests on this branch and went over the code briefly, and it doesn't seem to be using the cache at all now(?). I don't see where you're populating Dialer.dnsCache, so all my requests hit the DNS server regardless of the config.

I'll review this again once it's more fleshed out. For now only the dnsmessage comment seems like a blocker.

Comment on lines +85 to +92
v4: &dnsCache{
RWMutex: sync.RWMutex{},
cache: make(map[string]*cacheRecord),
},
v6: &dnsCache{
RWMutex: sync.RWMutex{},
cache: make(map[string]*cacheRecord),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are separate caches needed for v4 and v6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We get separate requests for them so it was easier to have them separate

)

// Message is a DNS message.
type Message struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be using this message implementation. It seems to be an old copy of the one currently used in stdlib, and we should rely on that one instead. This probably means that we can't use benburkert/dns as a library and will need to re-implement what we need ourselves.

@imiric
Copy link
Contributor

imiric commented Dec 2, 2020

Closing as discussed, see #1751.

@imiric imiric closed this Dec 2, 2020
@mstoykov mstoykov deleted the fix/726-738-dns-cache-refresh-impr branch February 3, 2021 12:45
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.

2 participants