Resolve DNS to IP & IPv6 address support #332
Conversation
zhenlineo
force-pushed
the
1.2-dns-to-ips
branch
from
dcbc6f4 to
e829799
Compare
There was a problem hiding this comment.
@zhenlineo review round completed. Do you think it is possible to add a boltkit test with some fake resolver to verify this feature end to end?
| // Don't remove it from the set of routers, since that might mean we lose our ability to re-discover, | ||
| // just remove it from the set of readers and writers, so that we don't use it for actual work without | ||
| // performing discovery first. | ||
| routers.remove( address ); |
There was a problem hiding this comment.
This #forget() method is also called on connection errors. Do we really want to drop routers here? I'd vote for not changing this and keeping #removeRouter().
There was a problem hiding this comment.
What's the point to keep in routing table? they were there to protect from running out of routers. Now we no longer have the problem with dns
| import org.neo4j.driver.internal.net.BoltServerAddress; | ||
| import org.neo4j.driver.v1.exceptions.ServiceUnavailableException; | ||
|
|
||
| public interface DnsResolver |
There was a problem hiding this comment.
Could we maybe have HostNameResolver interface and then DnsResolver as it's implementation?
| try | ||
| { | ||
| InetAddress[] ipAddresses = InetAddress.getAllByName( initialRouter.host() ); | ||
| Set<BoltServerAddress> addresses = new HashSet<>( ipAddresses.length ); |
There was a problem hiding this comment.
Specified like this initial capacity might still cause HashSet to resize because of default load factor or 0.75. Maybe not bother with initial size at all? :)
| } | ||
| catch ( UnknownHostException e ) | ||
| { | ||
| throw new ServiceUnavailableException( |
There was a problem hiding this comment.
What will happen if seed uri is already an IP address? Will UnknownHostException be thrown?
What do you think about just logging failure here and returning Collections.singleton(initialRouter)?
There was a problem hiding this comment.
IP to IP is not a problem, the test include using 127.0.0.1
| int size = routingTable.routerSize(); | ||
| Set<BoltServerAddress> triedServers = new HashSet<>( size ); |
There was a problem hiding this comment.
HashSet's initial capacity might be tricky here as well.
| return rediscovery.lookupClusterComposition( connections, routingTable ); | ||
| } | ||
|
|
||
| private static DnsResolver directMapProvider = new DnsResolver() |
There was a problem hiding this comment.
Maybe introduce a private static class for this or move it to the top of the class? It is just a bit strange to see field in the middle of the class.
| @Override | ||
| public Set<BoltServerAddress> resolve( BoltServerAddress initialRouter ) | ||
| { | ||
| Set<BoltServerAddress> directMap = new HashSet<>( 1 ); |
There was a problem hiding this comment.
Could probably use Collections.singleton( initialRouter ) here
There was a problem hiding this comment.
No, I cannot return any singlention set as it does not support removeAll method
zhenlineo
force-pushed
the
1.2-dns-to-ips
branch
from
2571169 to
3e67efb
Compare
|
@lutovich All boltkit tests that creates a real driver already go via DnsResolver |
zhenlineo
force-pushed
the
1.2-dns-to-ips
branch
from
3e67efb to
ea2cebd
Compare
zhenlineo
force-pushed
the
1.2-dns-to-ips
branch
from
ea2cebd to
9a6ca7b
Compare
No description provided.