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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
