-
Notifications
You must be signed in to change notification settings - Fork 34
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
Support unicast query and answer #176
Conversation
Blocked on #173 |
e9fa688
to
f593090
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #176 +/- ##
==========================================
+ Coverage 60.92% 62.14% +1.22%
==========================================
Files 1 1
Lines 412 597 +185
==========================================
+ Hits 251 371 +120
- Misses 125 170 +45
- Partials 36 56 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
f56a681
to
f1ac998
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, small comments to consider
for i := 0; i < numReaders; i++ { | ||
<-readerStarted | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could achieve something similar with sync.waitGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I was thinking that but didn't feel like bothering with the race condition panics around waitgroups
Also, should consider adding additional unit tests |
f1ac998
to
7bc4c3a
Compare
278c137
to
6b73f8d
Compare
I'd love to add more unit tests, but I'm not really sure how to do that succinctly without getting very in the weeds with capturing the DNS packets or injecting in the unicast sockets. I'm open to suggestions for future PRs though! |
This also ends up internally supporting a good chunk of IPv6