Allow users to specify DNS servers to use#31
Conversation
rculbertson
force-pushed
the
ryan/specify-dns-servers
branch
from
52d55b2 to
cf2a585
Compare
pettermahlen
left a comment
pettermahlen
left a comment
There was a problem hiding this comment.
The change looks good overall - the test case doesn't seem to actually be testing what it claims to do, though? It mostly just validates that the builder method exists. Something that would be great is to document the format of the servers list. I notice there's not a lot of javadoc to date, but a javadoc comment on the new builder method describing what it does and how to use it would be great.
| .newBuilder() | ||
| .servers(ImmutableList.of(server)) | ||
| .build(); | ||
| assertThat(resolver.resolve("_spotify-client._tcp.spotify.com").isEmpty(), is(false)); |
There was a problem hiding this comment.
This test doesn't actually test that the given resolver is used, does it?
There was a problem hiding this comment.
You're correct that it doesn't verify that specified server was actually used. But it's invoking the resolve method and testing that code path, making sure it returns successfully. I'm not sure how to test that the resolver actually used the specified server.
This PR modifies DnsSrvResolver so that each instance can be configured to use specific DNS servers. This PR also makes it so that timeouts are configured per instance, instead of globally.
rculbertson
force-pushed
the
ryan/specify-dns-servers
branch
from
cf2a585 to
a92ba0b
Compare
|
Good suggestion about javadoc. I added some to the builder method. |
|
Thanks @pettermahlen! |
2 participants
This PR modifies DnsSrvResolver to that each instance can be configured to use
specific DNS servers. This PR also makes it so that timeouts are configured per
instance, instead of globally.