[GitHub] [geode] aaronlindsey commented on a change in pull request #6938: GEODE-9666: Avoid caching InetSocketAddress

[GitBox <gi...@apache.org>]

aaronlindsey commented on a change in pull request #6938:
URL: https://github.com/apache/geode/pull/6938#discussion_r728295268



##########
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:
       Thanks for your feedback, @Bill!
   
   > 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.
   
   Yes, this was exactly my thinking as well 👍 
   
   As far as the performance concern from constructing more objects than necessary, I agree we have performance tests which should help catch any degradation. Also, I believe that Geode 1.13 and 1.14 shipped without this caching behavior (in them getSocketInetAddress() would always return a new InetSocketAddress when HostAndPort has an unresolved address). So I would expect no change in performance because of this change compared to 1.14.




-- 
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: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org