anmolnar commented on code in PR #2320: URL: https://github.com/apache/zookeeper/pull/2320#discussion_r2433725664
########## zookeeper-server/src/main/java/org/apache/zookeeper/client/HostConnectionManager.java: ########## @@ -0,0 +1,391 @@ +/* + * 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.zookeeper.client; + +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.UnknownHostException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Random; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * HostConnectionManager handles the complex connection management and reconfiguration logic Review Comment: I'd rather explain the connection management logic in detail in Javadoc. "handles complex logic" is meaningless. ########## zookeeper-server/src/main/java/org/apache/zookeeper/client/DnsSrvHostProvider.java: ########## @@ -0,0 +1,383 @@ +/* + * 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.zookeeper.client; + +import java.io.IOException; +import java.net.InetSocketAddress; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Objects; +import java.util.Timer; +import java.util.TimerTask; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.zookeeper.common.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.xbill.DNS.Lookup; +import org.xbill.DNS.Name; +import org.xbill.DNS.Record; +import org.xbill.DNS.SRVRecord; +import org.xbill.DNS.Type; + +/** + * DNS SRV-based HostProvider that dynamically resolves host port names from DNS SRV records. + * + * <p>This implementation periodically refreshes the server list by querying DNS SRV records + * and uses HostConnectionManager for all connection management and reconfiguration logic.</p> + * + * + * <p><strong>Two-Phase Update Strategy:</strong></p> + * <ul> + * <li><strong>Phase 1 (Background):</strong> Timer thread detects DNS changes without blocking</li> + * <li><strong>Phase 2 (Connection-time):</strong> Changes applied during actual connection attempts</li> + * </ul> + */ [email protected] +public final class DnsSrvHostProvider implements HostProvider { + + public interface DnsResolver { + /** + * Performs a DNS SRV lookup for the specified name. + * + * @param name the DNS name to look up + * @return array of SRV records, empty array if no records found + * @throws IOException if DNS lookup encounters an error + */ + SRVRecord[] lookupSrvRecords(String name) throws IOException; + } + + private static final Logger LOG = LoggerFactory.getLogger(DnsSrvHostProvider.class); + + private final String dnsName; + private final DnsResolver dnsResolver; + private final long refreshIntervalMs; + private final Timer dnsRefreshTimer; + private HostConnectionManager connectionManager; + + // Track the current connected host for accurate load balancing decisions + private final AtomicReference<InetSocketAddress> currentConnectedHost = new AtomicReference<>(); + // Track the previous server list to detect changes + private volatile List<InetSocketAddress> previousServerList = new ArrayList<>(); + private volatile List<InetSocketAddress> latestServerList = null; + private final AtomicBoolean serverListChanged = new AtomicBoolean(false); + + /** + * Constructs a DnsSrvHostProvider with the given DNS name. + * + * @param dnsName the DNS name to query for SRV records + * @throws IllegalArgumentException if dnsName is null or empty or invalid + */ + public DnsSrvHostProvider(final String dnsName) { + this(dnsName, System.currentTimeMillis() ^ dnsName.hashCode()); + } + + /** + * Constructs a DnsSrvHostProvider with deterministic randomness seed + * + * @param dnsName the DNS name to query for SRV records + * @param randomnessSeed seed for randomization + * @throws IllegalArgumentException if dnsName is null or empty or invalid + */ + DnsSrvHostProvider(final String dnsName, final long randomnessSeed) { + this(dnsName, randomnessSeed, new DefaultDnsResolver()); + } + + /** + * Constructs a DnsSrvHostProvider with the given DNS name, randomization seed and DNS resolver + * + * @param dnsName the DNS name to query for SRV records + * @param randomnessSeed seed for randomization + * @param dnsResolver custom DNS resolver for testing + * @throws IllegalArgumentException if dnsName is null or empty or invalid + */ + public DnsSrvHostProvider(final String dnsName, final long randomnessSeed, final DnsResolver dnsResolver) { + this.dnsName = validateDnsName(dnsName); + this.dnsResolver = dnsResolver; + this.refreshIntervalMs = validateRefreshInterval(); + this.dnsRefreshTimer = initializeDnsRefreshTimer(); + + init(randomnessSeed); + } + + @Override + public int size() { + return connectionManager.size(); + } + + @Override + public InetSocketAddress next(long spinDelay) { + applyPendingServerListUpdate(); + return connectionManager.next(spinDelay); + } + + @Override + public void onConnected() { + currentConnectedHost.set(connectionManager.getServerAtCurrentIndex()); + connectionManager.onConnected(); + } + + @Override + public boolean updateServerList(Collection<InetSocketAddress> serverAddresses, InetSocketAddress currentHost) { + return connectionManager.updateServerList(serverAddresses, currentHost); + } + + @Override + public void close() { + if (dnsRefreshTimer != null) { + dnsRefreshTimer.cancel(); + } + } + + @Override + public ConnectionType getSupportedConnectionType() { + return ConnectionType.DNS_SRV; + } + + /** + * Validates and processes the DNS name. + * + * @param dnsName the DNS name to validate + * @return the trimmed and validated DNS name + * @throws IllegalArgumentException if dnsName is null, empty, or invalid + */ + private String validateDnsName(final String dnsName) { + if (StringUtils.isBlank(dnsName)) { + throw new IllegalArgumentException("DNS name cannot be null or empty"); + } + + final String trimmedDnsName = dnsName.trim(); + // Ensure DNS name is absolute (ends with dot) for proper DNS resolution + final String absoluteDnsName = trimmedDnsName.endsWith(".") ? trimmedDnsName : trimmedDnsName + "."; + + try { + Name.fromString(absoluteDnsName); Review Comment: You can do a lookup immediately in order to validate that the DNS name has valid SRV record. ########## zookeeper-server/src/main/java/org/apache/zookeeper/client/DnsSrvHostProvider.java: ########## @@ -0,0 +1,383 @@ +/* + * 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.zookeeper.client; + +import java.io.IOException; +import java.net.InetSocketAddress; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Objects; +import java.util.Timer; +import java.util.TimerTask; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.zookeeper.common.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.xbill.DNS.Lookup; +import org.xbill.DNS.Name; +import org.xbill.DNS.Record; +import org.xbill.DNS.SRVRecord; +import org.xbill.DNS.Type; + +/** + * DNS SRV-based HostProvider that dynamically resolves host port names from DNS SRV records. + * + * <p>This implementation periodically refreshes the server list by querying DNS SRV records + * and uses HostConnectionManager for all connection management and reconfiguration logic.</p> + * + * + * <p><strong>Two-Phase Update Strategy:</strong></p> + * <ul> + * <li><strong>Phase 1 (Background):</strong> Timer thread detects DNS changes without blocking</li> + * <li><strong>Phase 2 (Connection-time):</strong> Changes applied during actual connection attempts</li> + * </ul> + */ [email protected] +public final class DnsSrvHostProvider implements HostProvider { + + public interface DnsResolver { + /** + * Performs a DNS SRV lookup for the specified name. + * + * @param name the DNS name to look up + * @return array of SRV records, empty array if no records found + * @throws IOException if DNS lookup encounters an error + */ + SRVRecord[] lookupSrvRecords(String name) throws IOException; + } + + private static final Logger LOG = LoggerFactory.getLogger(DnsSrvHostProvider.class); + + private final String dnsName; + private final DnsResolver dnsResolver; + private final long refreshIntervalMs; + private final Timer dnsRefreshTimer; + private HostConnectionManager connectionManager; + + // Track the current connected host for accurate load balancing decisions + private final AtomicReference<InetSocketAddress> currentConnectedHost = new AtomicReference<>(); + // Track the previous server list to detect changes + private volatile List<InetSocketAddress> previousServerList = new ArrayList<>(); + private volatile List<InetSocketAddress> latestServerList = null; + private final AtomicBoolean serverListChanged = new AtomicBoolean(false); + + /** + * Constructs a DnsSrvHostProvider with the given DNS name. + * + * @param dnsName the DNS name to query for SRV records + * @throws IllegalArgumentException if dnsName is null or empty or invalid + */ + public DnsSrvHostProvider(final String dnsName) { + this(dnsName, System.currentTimeMillis() ^ dnsName.hashCode()); + } + + /** + * Constructs a DnsSrvHostProvider with deterministic randomness seed + * + * @param dnsName the DNS name to query for SRV records + * @param randomnessSeed seed for randomization + * @throws IllegalArgumentException if dnsName is null or empty or invalid + */ + DnsSrvHostProvider(final String dnsName, final long randomnessSeed) { + this(dnsName, randomnessSeed, new DefaultDnsResolver()); + } + + /** + * Constructs a DnsSrvHostProvider with the given DNS name, randomization seed and DNS resolver + * + * @param dnsName the DNS name to query for SRV records + * @param randomnessSeed seed for randomization + * @param dnsResolver custom DNS resolver for testing + * @throws IllegalArgumentException if dnsName is null or empty or invalid + */ + public DnsSrvHostProvider(final String dnsName, final long randomnessSeed, final DnsResolver dnsResolver) { + this.dnsName = validateDnsName(dnsName); + this.dnsResolver = dnsResolver; + this.refreshIntervalMs = validateRefreshInterval(); + this.dnsRefreshTimer = initializeDnsRefreshTimer(); + + init(randomnessSeed); + } + + @Override + public int size() { + return connectionManager.size(); + } + + @Override + public InetSocketAddress next(long spinDelay) { + applyPendingServerListUpdate(); + return connectionManager.next(spinDelay); + } + + @Override + public void onConnected() { + currentConnectedHost.set(connectionManager.getServerAtCurrentIndex()); + connectionManager.onConnected(); + } + + @Override + public boolean updateServerList(Collection<InetSocketAddress> serverAddresses, InetSocketAddress currentHost) { + return connectionManager.updateServerList(serverAddresses, currentHost); + } + + @Override + public void close() { + if (dnsRefreshTimer != null) { + dnsRefreshTimer.cancel(); + } + } + + @Override + public ConnectionType getSupportedConnectionType() { + return ConnectionType.DNS_SRV; + } + + /** + * Validates and processes the DNS name. + * + * @param dnsName the DNS name to validate + * @return the trimmed and validated DNS name + * @throws IllegalArgumentException if dnsName is null, empty, or invalid + */ + private String validateDnsName(final String dnsName) { + if (StringUtils.isBlank(dnsName)) { + throw new IllegalArgumentException("DNS name cannot be null or empty"); + } + + final String trimmedDnsName = dnsName.trim(); + // Ensure DNS name is absolute (ends with dot) for proper DNS resolution + final String absoluteDnsName = trimmedDnsName.endsWith(".") ? trimmedDnsName : trimmedDnsName + "."; + + try { + Name.fromString(absoluteDnsName); + } catch (final Exception e) { + throw new IllegalArgumentException("Invalid DNS name format: " + absoluteDnsName, e); + } + return absoluteDnsName; + } + + /** + * Validates and returns the refresh interval + * + * @return the validated refresh interval in milliseconds + */ + private long validateRefreshInterval() { + final long defaultInterval = ZKClientConfig.DNS_SRV_REFRESH_INTERVAL_MS_DEFAULT; + + final long interval = Long.getLong(ZKClientConfig.DNS_SRV_REFRESH_INTERVAL_MS, defaultInterval); + if (interval < 0) { + LOG.warn("Invalid refresh interval {}, using default {}", interval, defaultInterval); + return defaultInterval; + } + if (interval == 0) { + LOG.info("DNS refresh disabled (interval = 0) for {}", dnsName); + } + return interval; + } + + + /** + * Initializes the DNS refresh timer if refresh interval is greater than 0. + * + * @return the initialized Timer or null if refresh interval is 0 or negative + */ + private Timer initializeDnsRefreshTimer() { Review Comment: I don't think you need this internal caching thing at all in ZooKeeper. DNS has its own multi-layer caching logic in the IP stack of the operating system and at the DNS resolver servers. Just query the SRV entry every time you run out of available servers and DNS will take care of the rest. There's not much benefit in caching something which is already cached in the host's memory. -- 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]
