Skip to content

Conversation

li4wang
Copy link
Contributor

@li4wang li4wang commented Sep 27, 2025

The existing ZooKeeper client architecture relies on StaticHostProvider, which lacks auto service discovery capabilities and must wait for external mechanisms to push server list updates, either through manual configuration changes or reconfig notifications.

This PR provides a HostProvider implementation that performs dynamic service discovery based on DNS SRV record. The following is a summary of the changes:

  1. DnsSrvHostProvider: New HostProvider implementation that performs DNS SRV lookups to discover ZooKeeper servers
  2. HostConnectionManager: Extracted connection management and reconfiguration logic from StaticHostProvider into a reusable component
  3. StaticHostProvider Refactoring: Modified to use HostConnectionManager for consistent connection handling across providers
  4. ConnectionType: New enum that represents the type of connection resolutions
  5. Configuration: Added zookeeper.hostProvider.dnsSrvRefreshIntervalMs client property for DNS refresh intervals

Copy link
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are the changes of StaticHostProvider?

final String absoluteDnsName = trimmedDnsName.endsWith(".") ? trimmedDnsName : trimmedDnsName + ".";

try {
Name.fromString(absoluteDnsName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do a lookup immediately in order to validate that the DNS name has valid SRV record.

*
* @return the initialized Timer or null if refresh interval is 0 or negative
*/
private Timer initializeDnsRefreshTimer() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import org.slf4j.LoggerFactory;

/**
* HostConnectionManager handles the complex connection management and reconfiguration logic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather explain the connection management logic in detail in Javadoc. "handles complex logic" is meaningless.

<scope>compile</scope>
</dependency>
<dependency>
<groupId>dnsjava</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there standard Java API for this which is not third party?

* HostConnectionManager handles the complex connection management and reconfiguration logic
*/
@InterfaceAudience.Private
public final class HostConnectionManager {
Copy link
Contributor

@anmolnar anmolnar Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be the common abstract class for both implementations. Doing it with inheritance might be simpler than this composition where you just forward the calls to Connection Manager.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants