Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

PR for Duplicated UDP responses from DNS confuse LookupClient #140 #142

Merged
merged 3 commits into from
Jan 26, 2022
Merged

PR for Duplicated UDP responses from DNS confuse LookupClient #140 #142

merged 3 commits into from
Jan 26, 2022

Conversation

JamesKovacs
Copy link
Contributor

The following PR is to resolve the issue reported in #140.

@MichaCo
Copy link
Owner

MichaCo commented Jan 23, 2022

Hi @JamesKovacs
I think the implementation itself looks reasonable (will review it a bit more now)

The unit test, especially Lookup_XidMismatch seems a bit flaky though: The 2 tests with mismatchResponses = 10 fail for me locally all the time.

MichaCo
MichaCo requested changes Jan 23, 2022
View reviewed changes
Copy link
Owner

@MichaCo MichaCo left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just 2 minor things

test/DnsClient.Tests/LookupTest.cs Outdated
[InlineData(0, false)]
[InlineData(1, false)]
[InlineData(3, false)]
[InlineData(10, false)]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
[InlineData(10, false)]
//[InlineData(10, false)]

test/DnsClient.Tests/LookupTest.cs Outdated
[InlineData(0, true)]
[InlineData(1, true)]
[InlineData(3, true)]
[InlineData(10, true)]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
[InlineData(10, true)]
//[InlineData(10, true)]

Not sure why those 2 variants fail, at least when run under .NET47x.
Let's just comment it out for now?

src/DnsClient/DnsMessageHandler.cs Outdated Show resolved Hide resolved
@JamesKovacs
Copy link
Contributor Author

Thanks for the feedback. I've addressed your comments and will push another commit to the branch shortly.

@JamesKovacs
Copy link
Contributor Author

Changes pushed and ready for review. Thanks again!

@MichaCo MichaCo merged commit d56cb12 into MichaCo:dev Jan 26, 2022
@MichaCo
Copy link
Owner

MichaCo commented Jan 26, 2022

The tests failed during build. I'll fix that later, seems not really related to your changes.
I have to finish one more change and then planning to release this end of the week I think.

Thanks again for your help!

@djj0809
Copy link

djj0809 commented Jan 30, 2022

@MichaCo Looking forward to the new version.

We have been seeing DNS resolution failure in one of our cluster from time to time but was not able to identity the root cause, thanks @JamesKovacs !

@euyuil euyuil mentioned this pull request Jan 30, 2022
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@MichaCo MichaCo MichaCo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JamesKovacs @MichaCo @djj0809 @BorisDog