Bill commented on a change in pull request #6308: URL: https://github.com/apache/geode/pull/6308#discussion_r617974408
########## File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/HostAddress.java ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.distributed.internal.membership.api; + +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.util.Objects; + +import org.apache.commons.validator.routines.InetAddressValidator; + +import org.apache.geode.distributed.internal.tcpserver.HostAndPort; + +/** + * HostAddress is a holder of a host name/address. It is the primary + * way to specify a host address that may or may not have been resolved to an InetAddress. + * + * @See HostAddress which can hold both a host name and a port + */ +public class HostAddress { + + private InetSocketAddress address; + + public HostAddress(String hostName) { + if (hostName == null) { + address = new InetSocketAddress(0); + } else if (InetAddressValidator.getInstance().isValid(hostName)) { + // numeric address - use as-is + address = new InetSocketAddress(hostName, 0); + } else { + // non-numeric address - resolve hostname when needed + address = InetSocketAddress.createUnresolved(hostName, 0); + } + } + + public HostAddress(InetAddress address) { + if (address == null) { + throw new IllegalArgumentException("null parameters are not allowed"); + } + this.address = new InetSocketAddress(address, 0); + } + + public HostAddress(HostAndPort host) { + this.address = host.getSocketInetAddress(); + } + + /** + * Returns an InetSocketAddress for this host and port. An attempt is made to resolve the + * host name but if resolution fails an unresolved InetSocketAddress is returned. This return + * value will not hold an InetAddress, so calling getAddress() on it will return null. + */ + private InetSocketAddress getInetSocketAddress() { + if (address.isUnresolved()) { + // note that this leaves the InetAddress null if the hostname isn't resolvable + return new InetSocketAddress(address.getHostString(), address.getPort()); + } else { + return this.address; + } + } + + public String getHostName() { + String hostString = address.getHostString(); + if (hostString != null) { + return hostString; + } Review comment: I looked at `InetSocketAddress` (docs and implementation) and it looks like `getHostName()` already avoids the reverse lookup if the host string is already known. So I believe lines 73-76 are superfluous. ########## File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/api/HostAddress.java ########## @@ -0,0 +1,106 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ +package org.apache.geode.distributed.internal.membership.api; + +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.util.Objects; + +import org.apache.commons.validator.routines.InetAddressValidator; + +import org.apache.geode.distributed.internal.tcpserver.HostAndPort; + +/** + * HostAddress is a holder of a host name/address. It is the primary + * way to specify a host address that may or may not have been resolved to an InetAddress. + * + * @See HostAddress which can hold both a host name and a port + */ +public class HostAddress { + + private InetSocketAddress address; + + public HostAddress(String hostName) { + if (hostName == null) { + address = new InetSocketAddress(0); + } else if (InetAddressValidator.getInstance().isValid(hostName)) { + // numeric address - use as-is + address = new InetSocketAddress(hostName, 0); + } else { + // non-numeric address - resolve hostname when needed + address = InetSocketAddress.createUnresolved(hostName, 0); + } + } + + public HostAddress(InetAddress address) { + if (address == null) { + throw new IllegalArgumentException("null parameters are not allowed"); + } + this.address = new InetSocketAddress(address, 0); + } + + public HostAddress(HostAndPort host) { + this.address = host.getSocketInetAddress(); + } + + /** + * Returns an InetSocketAddress for this host and port. An attempt is made to resolve the + * host name but if resolution fails an unresolved InetSocketAddress is returned. This return + * value will not hold an InetAddress, so calling getAddress() on it will return null. + */ + private InetSocketAddress getInetSocketAddress() { + if (address.isUnresolved()) { + // note that this leaves the InetAddress null if the hostname isn't resolvable + return new InetSocketAddress(address.getHostString(), address.getPort()); Review comment: Shouldn't `HostAddress` cache this value before returning it? Failing to do so causes us to resolve the hostname every single time the socket address is needed. If this is a problem here, it's also a problem in the sister-class (or perhaps mother-class) `HostAndPort`—it also fails to cache the value. Also I recommend calling this method `getSocketInetAddress` to match the one in `HostAndPort`. -- 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. For queries about this service, please contact Infrastructure at: [email protected]
