JAVA-3131: Add #retrieve method to EndPoint for when caller does not …#1735
JAVA-3131: Add #retrieve method to EndPoint for when caller does not …#1735hhughes wants to merge 1 commit intoapache:4.xfrom
Conversation
hhughes
force-pushed
the
JAVA-3131
branch
3 times, most recently
from
5305d76 to
989f1a5
Compare
…need the endpoint to be proactively resolved Refactor existing usages of EndPoint#resolve to use retrieve when resolved ip addresses are not needed.
absurdfarce
left a comment
absurdfarce
left a comment
There was a problem hiding this comment.
A few minor things but overall I very much like where this change is headed!
| @NonNull | ||
| default SocketAddress retrieve() { | ||
| return resolve(); | ||
| } |
There was a problem hiding this comment.
I'm wondering if it makes sense to provide a default implementation for the new method. It's unlikely we'll be adding a new endpoint type anytime soon but failing to distinguish between "look up the addresses again" and "give me the addresses you got last time you looked them up" is what got us into this situation in the first place. I kinda feel like having this as a default might lend itself to landing back in exactly that situation.
| @NonNull | ||
| @Override | ||
| public InetSocketAddress resolve() { | ||
| return retrieve(); |
There was a problem hiding this comment.
Nit: save yourself the method call overhead here and just return address directly... ?
| try { | ||
| int[] comp = Arrays.stream(addr.split("\\.")).mapToInt(Integer::parseInt).toArray(); | ||
| return InetAddress.getByAddress( | ||
| new byte[] {(byte) comp[0], (byte) comp[1], (byte) comp[2], (byte) comp[3]}); |
There was a problem hiding this comment.
Is there a particular reason to jump through the splitting/to Int/toArray ops here rather than just using InetAddress.getByName()? I don't think it does a reverse lookup if you just give it a string containing an IP address but I could be wrong.
There was a problem hiding this comment.
Sorry, I am confused. Could you pls explain more?
InetAddress.getByAddress does not do reverse lookup. InetAddress.getByName will look it up. We don't want it to look up. Therefore, I think InetAddress.getByAddress is the one we should use. :)
|
|
||
| @Override | ||
| public InetSocketAddress retrieve() { | ||
| return proxyAddress; |
There was a problem hiding this comment.
Don't we want to cache the last value returned by resolve() and return that here instead?
|
Work taken over to #1919 |
|
Replaced by #1919. Review comments will be handled there. |
3 participants
…need the endpoint to be proactively resolved
Refactor existing usages of EndPoint#resolve to use retrieve when resolved ip addresses are not needed.