Bill commented on a change in pull request #6938:
URL: https://github.com/apache/geode/pull/6938#discussion_r727554745
##########
File path:
geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/InetSocketWrapper.java
##########
@@ -77,13 +76,6 @@ public boolean equals(Object o) {
* value will not hold an InetAddress, so calling getAddress() on it will
return null.
*/
public InetSocketAddress getSocketInetAddress() {
- if (attemptedToResolve == null) {
- attemptedToResolve = basicGetInetSocketAddress();
- }
- return attemptedToResolve;
- }
-
- private InetSocketAddress basicGetInetSocketAddress() {
Review comment:
@aaronlindsey I understand why caching this name resolution is bad,
especially in the managed environment. The answer in this PR: never cache,
worries me tho because now every call to `getSocketInetAddress()` on an object
constructed with a hostname-or-FQDN `hostName` parameter will 1) perform a DNS
lookup and 2) construct a brand new InetSocketAddress().
I suppose it should be possible to mitigate these concerns with performance
testing. Certainly the second concern. DNS latency is highly
environment-specific, so the first concern might be a little harder to address
definitively.
Then again JDKs have system properties to control how long the JDK itself
caches successful/unsuccessful name resolution attempts. So we have the whole
DNS infrastructure itself we can tune, and we have JDK settings we can further
tune. So concern (1) should be addressable via tuning if necessary.
So yeah, I think you've got it right. ✓
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
