feat: multi-address DNS resolution for contact points and connections (DRIVER-201) - #890
feat: multi-address DNS resolution for contact points and connections (DRIVER-201)#890nikagra wants to merge 29 commits into
Conversation
…VER-201) newControlReconnectionQueryPlan() now creates copies of the original contact-point nodes (with their unresolved hostname endpoints) instead of synthetic nodes with resolved IPs. This ensures the control channel carries the hostname endpoint, which is preserved in metadata after topology refresh. DNS expansion for connection fallback is handled by ChannelFactory (PR scylladb#890), so the control-reconnection path does not need to inject resolved-IP nodes into the query plan. Also adds getContactPoints() stub back to LoadBalancingPolicyWrapperTest so tests that cover the control-reconnect path continue to pass.
Before-init query plan now uses getContactPoints() (original unresolved hostname nodes) instead of getResolvedContactPoints(). The DNS expansion to all IPs happens at the ChannelFactory level (PR scylladb#890), so expanding here was redundant and broke should_connect_with_mocked_hostname by replacing hostname endpoints with resolved-IP endpoints. Also remove the should_connect_when_first_dns_entry_is_non_responsive integration test from this PR; it belongs in PR scylladb#890 where ChannelFactory expansion actually enables it to pass.
There was a problem hiding this comment.
Pull request overview
Part 2/2 of DRIVER-201: extends the EndPoint API and ChannelFactory so that a hostname mapping to multiple IPs is tried address-by-address at the connection layer, instead of only the first IP. The EndPoint.resolve() method is deprecated in favor of a new resolveAll() default method; DefaultEndPoint, SniEndPoint, and ClientRoutesEndPoint override it; ChannelFactory.connect() now iterates over candidates and only fails when all are exhausted, while keeping protocol-version downgrade scoped to a single address.
Changes:
- Add
EndPoint.resolveAll()(default impl delegating to deprecatedresolve()); override inDefaultEndPoint,SniEndPoint,ClientRoutesEndPoint. - Rework
ChannelFactory.connect()intotryNextCandidate/connectToAddressso per-address failures fall back to the next IP while protocol-version downgrades stay scoped to one address. - Add unit tests for
DefaultEndPoint.resolveAll()and a newSniEndPointTest.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java | Deprecates resolve(); adds default resolveAll() method. |
| core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java | Overrides resolveAll() using InetAddress.getAllByName with single-address fallback. |
| core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java | Overrides resolveAll() returning one address per sorted A-record. |
| core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java | Overrides resolveAll() to wrap the single topology-monitor address. |
| core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java | Adds candidate-iteration and per-address protocol-negotiation methods. |
| core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java | New tests for resolveAll() (resolved, unresolved expansion, unresolvable fallback). |
| core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java | New test class covering SNI resolveAll() happy path, unresolvable host, and resolve() sanity check. |
Comments suppressed due to low confidence (1)
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java:303
- When
connectToAddressfails withUnsupportedProtocolVersionException.forNegotiation(i.e. all protocol downgrades exhausted),tryNextCandidatewill treat this like any other per-address failure and try the next IP, even though the protocol-negotiation failure is a server-wide condition that will recur on every other IP of the same node. This also reuses the sharedattemptedVersionsCopyOnWriteArrayListacross candidates, so on each subsequent address the downgrade loop re-attempts the same protocol versions and adds duplicate entries, and the final exception ultimately reported will list each version multiple times. Consider distinguishing non-address-specific failures (UnsupportedProtocolVersionException, authentication errors, etc.) and short-circuiting the candidate loop in those cases.
perAddressFuture.whenComplete(
(channel, error) -> {
if (error == null) {
resultFuture.complete(channel);
} else if (index + 1 < candidates.length) {
LOG.debug(
"[{}] Failed to connect to {} ({}), trying next address",
logPrefix,
candidate,
error.getMessage());
tryNextCandidate(
endPoint,
shardingInfo,
shardId,
options,
nodeMetricUpdater,
currentVersion,
isNegotiating,
attemptedVersions,
resultFuture,
candidates,
index + 1);
} else {
// Note: might be completed already if the failure happened in initializer()
resultFuture.completeExceptionally(error);
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
05553f3 to
f631971
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Sequence Diagram(s)sequenceDiagram
participant ChannelFactory
participant EndPoint
participant tryNextCandidate
participant connectToAddress
participant resultFuture
ChannelFactory->>EndPoint: resolveAll()
EndPoint-->>ChannelFactory: SocketAddress[] candidates
ChannelFactory->>tryNextCandidate: attempt candidate at index 0
tryNextCandidate->>connectToAddress: connect using perAddressFuture
alt connection succeeds
connectToAddress-->>tryNextCandidate: DriverChannel
tryNextCandidate->>resultFuture: complete successfully
else connection or negotiation fails
connectToAddress-->>tryNextCandidate: complete perAddressFuture exceptionally
tryNextCandidate->>tryNextCandidate: attempt next candidate
end
tryNextCandidate->>resultFuture: fail after all candidates
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
f631971 to
860a34d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java`:
- Around line 222-242: The code calls endPoint.resolveAll() and passes the
resulting candidates array into tryNextCandidate() which immediately indexes
candidates[0]; guard against null or empty results by validating the output of
endPoint.resolveAll()—if it returns null or candidates.length == 0, complete
resultFuture exceptionally (or create a specific error) and return; otherwise
call tryNextCandidate(...) with the non-empty candidates. Update the block
around resolveAll(), candidates, and the call to tryNextCandidate() to perform
this check and fail fast via resultFuture.completeExceptionally when
appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ad3d5b5-6473-4c88-8777-93861f5de639
📒 Files selected for processing (12)
core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.javacore/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.javacore/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java
860a34d to
a6d0e48
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java (1)
37-37: ⚡ Quick winConsider adding test coverage for resolveAll() throwing an exception.
The
ChannelFactory.connect()implementation includes a catch block for exceptions thrown byresolveAll()(see context snippet 1, line 232). Adding a third test case where the mockedEndPoint.resolveAll()throws an exception (e.g.,UnknownHostException) would ensure all three defensive paths are tested:
- ✓ Returns null (covered)
- ✓ Returns empty array (covered)
- ✗ Throws exception (not covered)
📋 Suggested test case
`@Test` public void should_fail_future_when_resolve_all_throws_exception() { // Given when(defaultProfile.isDefined(DefaultDriverOption.PROTOCOL_VERSION)).thenReturn(false); when(protocolVersionRegistry.highestNonBeta()).thenReturn(DefaultProtocolVersion.V4); ChannelFactory factory = newChannelFactory(); EndPoint badEndPoint = mock(EndPoint.class); RuntimeException testException = new RuntimeException("DNS lookup failed"); when(badEndPoint.resolveAll()).thenThrow(testException); // When CompletionStage<DriverChannel> channelFuture = factory.connect( badEndPoint, null, null, DriverChannelOptions.DEFAULT, NoopNodeMetricUpdater.INSTANCE); // Then – future must complete exceptionally with the thrown exception assertThatStage(channelFuture) .isFailed(e -> assertThat(e).isSameAs(testException)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java` at line 37, Add a third test in ChannelFactoryResolveAllGuardTest that verifies ChannelFactory.connect() propagates exceptions thrown by EndPoint.resolveAll(): mock an EndPoint (e.g., badEndPoint) to throw a RuntimeException (or UnknownHostException) from resolveAll(), create the factory via newChannelFactory(), call factory.connect(badEndPoint, ...) with DriverChannelOptions.DEFAULT and NoopNodeMetricUpdater.INSTANCE, and assert the returned CompletionStage<DriverChannel> completes exceptionally with the same exception; this mirrors the existing tests for null/empty resolveAll() and targets the catch path in ChannelFactory.connect().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java`:
- Line 37: Add a third test in ChannelFactoryResolveAllGuardTest that verifies
ChannelFactory.connect() propagates exceptions thrown by EndPoint.resolveAll():
mock an EndPoint (e.g., badEndPoint) to throw a RuntimeException (or
UnknownHostException) from resolveAll(), create the factory via
newChannelFactory(), call factory.connect(badEndPoint, ...) with
DriverChannelOptions.DEFAULT and NoopNodeMetricUpdater.INSTANCE, and assert the
returned CompletionStage<DriverChannel> completes exceptionally with the same
exception; this mirrors the existing tests for null/empty resolveAll() and
targets the catch path in ChannelFactory.connect().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b702fd48-9ba7-4994-8bb9-351438fb02a8
📒 Files selected for processing (13)
core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.javacore/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.javacore/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java
✅ Files skipped from review due to trivial changes (5)
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java
- core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.java
- core/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.java
🚧 Files skipped from review as they are similar to previous changes (7)
- core/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java
- core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java
- core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java
a6d0e48 to
f9265b3
Compare
|
🤖: Valid nitpick. Added a third test |
f9265b3 to
4448119
Compare
4448119 to
1c8dfa2
Compare
|
Rebased this PR (Part 2/2) on top of #889 ( Also addressed the outstanding review feedback:
Previously-addressed items (Copilot / CodeRabbit) remain in place after the rebase: N×timeout Javadoc on Verified locally on JDK 11: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java:62
NETTY_ADMIN_SIZEonly configures the number of admin event-loop threads (DefaultDriverOption.java:807-811); it does not configure anAddressResolverGroup. This link gives users an incorrect way to identify or change the resolver. Refer to a customNettyOptionsbootstrap hook instead, or omit the configuration link.
* <p><b>Note on resolver:</b> DNS lookup is performed via {@link
* InetAddress#getAllByName(String)} on the calling thread, bypassing any custom Netty {@code
* AddressResolverGroup} configured via {@link
* com.datastax.oss.driver.api.core.config.DefaultDriverOption#NETTY_ADMIN_SIZE}. This is
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java`:
- Line 603: Update the public Javadoc for the reconnection-plan option in
TypedDriverOption to state that it appends DNS-expanded candidates returned by
getResolvedContactPoints(), rather than raw original contact points, and that
monitors which re-resolve addresses skip this behavior; retain the documented
default of true.
In
`@core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java`:
- Around line 147-153: Prevent blocking DNS resolution from query-plan creation
by moving MetadataManager.getResolvedContactPoints() off the caller thread or
introducing bounded caching before using its results. Apply the fix to the
BEFORE_INIT/DURING_INIT path in
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java:147-153
and the control-reconnect path in
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.java:164-184;
update core/src/main/resources/reference.conf:2321-2334 if needed so
fallback-to-original-contact-points is not enabled without bounded, non-blocking
resolution.
In `@core/src/main/resources/reference.conf`:
- Around line 2321-2334: The default for fallback-to-original-contact-points
must not enable the blocking DNS fallback path; change this configuration
default back to false while preserving the existing setting name and
documentation.
In
`@core/src/test/java/com/datastax/oss/driver/internal/core/metadata/MetadataManagerTest.java`:
- Around line 512-529: The test should enforce expansion to the complete DNS
result set, not merely verify that one resolved node exists. Update
should_expand_unresolved_hostname_to_all_ips to obtain
InetAddress.getAllByName("localhost"), compare the returned node count and
endpoint addresses against all expected addresses on port 9042, and retain the
resolved-address assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 648940a1-36ee-47f0-8f02-aff008723307
📒 Files selected for processing (29)
core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.javacore/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.javacore/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.javacore/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.javacore/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.javacore/src/main/java/com/datastax/oss/driver/api/core/session/SessionBuilder.javacore/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/helper/OptionalLocalDcHelper.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/CloudTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapper.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/MetadataManager.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.javacore/src/main/java/com/datastax/oss/driver/internal/core/metadata/TopologyMonitor.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/DefaultSslEngineFactory.javacore/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.javacore/src/main/resources/reference.confcore/src/test/java/com/datastax/dse/driver/internal/core/insights/InsightsClientTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapperTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/MetadataManagerTest.javacore/src/test/java/com/datastax/oss/driver/internal/core/metadata/SniEndPointTest.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/heartbeat/HeartbeatIT.javaintegration-tests/src/test/java/com/datastax/oss/driver/core/resolver/MockResolverIT.java
🚧 Files skipped from review as they are similar to previous changes (11)
- core/src/test/java/com/datastax/oss/driver/internal/core/channel/ChannelFactoryResolveAllGuardTest.java
- core/src/main/java/com/datastax/dse/driver/api/core/auth/DseGssApiAuthProviderBase.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesEndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/SniEndPoint.java
- core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java
- core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java
- core/src/main/java/com/datastax/dse/driver/internal/core/insights/InsightsClient.java
- core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPoint.java
- core/src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultEndPointTest.java
… (DRIVER-201) When RESOLVE_CONTACT_POINTS=false (the default) a hostname contact point was stored as a single unresolved InetSocketAddress, so the query plan tried only the first DNS IP. Keep contact points unresolved and expand each hostname to all its DNS IPs at query-plan time via MetadataManager.getResolvedContactPoints(), so the driver falls back to the next candidate when one IP is unreachable. Resolution is bounded, concurrent and best-effort. getResolvedContactPoints() runs on the admin event loop, where nothing should block, so each blocking InetAddress.getAllByName() call is offloaded to a cached daemon-thread pool and all unresolved hostnames are resolved concurrently against a single CONTACT_POINT_RESOLUTION_TIMEOUT deadline. A cached pool (rather than one shared thread) means each hostname resolves on its own thread, so one slow or blackholed lookup cannot starve the sibling contact points, nor the next reconnect that would otherwise queue behind it. If a hostname cannot be resolved or resolution times out, the original unresolved contact point is kept as-is rather than dropped, so the query plan is never emptier than the configured contact points and the address can still be resolved later at connection time (as it was before DNS expansion existed). This is an interim mitigation, superseded by scylladb#890's non-blocking EndPoint.resolveAll(). Default advanced.control-connection.reconnection.fallback-to-original-contact-points to true (no longer Experimental): it is the DNS re-resolution path on reconnect. Metadata nodes hold an already-resolved endpoint that is never re-resolved, so falling back to the original unresolved contact points re-expands the hostname to its current DNS IPs. Document that DNS-expanded contact points are IP-backed connection candidates that may be persisted in metadata, and that each synthetic endpoint retains the original hostname (built from the resolved InetAddress) so TLS peer host / SNI / hostname verification keep using the configured hostname. Gate the control-connection reconnection contact-point fallback behind a new TopologyMonitor.reresolvesNodeAddresses() (default false; true for the proxy-based ClientRoutesTopologyMonitor and CloudTopologyMonitor). Those monitors reach nodes through endpoints that already re-resolve on every connection attempt and maintain an authoritative node set, so appending raw contact points to their reconnection plan is unnecessary and could resurrect removed nodes (PrivateLink/Cloud regression safety). The reconnection plan also appends the contact points only once the load balancing policy is RUNNING, so the pre-init plan (already built from the resolved contact points) is not duplicated or re-resolved. Remove OptionalLocalDcHelper.checkLocalDatacenterCompatibility(): it warned when a contact point reported a different datacenter than the configured local DC. Since commit 12e6acb switched initial metadata refresh to hostId-only matching, contact-point nodes are never reused and their datacenter stays null; comparing a configured local DC against that null made the check fire as a false positive for every contact point whenever local-datacenter was set on the default profile, rather than surface a real mismatch. The node-based "configured local DC matches no node" warning (against discovered nodes whose datacenters are populated) is retained, so the only user-visible effect is that the spurious warning is no longer emitted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… (DRIVER-201) attemptedVersions was created once per connect() call and threaded unchanged through every candidate address tried by tryNextCandidate(). If candidate scylladb#1 exhausted protocol-downgrade negotiation before falling back to candidate scylladb#2, and scylladb#2 also exhausted negotiation, the final UnsupportedProtocolVersionException reported a version-history conflated from two different IPs. Construct a fresh list per candidate inside tryNextCandidate() instead of threading one down from connect(); connectToAddress()'s own downgrade-retry recursion (correctly scoped to a single address) is unaffected.
…esses javadoc (DRIVER-201) LoadBalancingPolicyWrapper.newControlReconnectionQueryPlan(): document the narrow window between the outer captured `state` read and newQueryPlan()'s own internal stateRef read -- a transition landing between them can skip the contact-point fallback for one reconnection attempt. Benign (no crash, no duplicate entries, self-corrects on the next attempt), but worth spelling out next to the existing "state is monotonic" reasoning. TopologyMonitor.reresolvesNodeAddresses(): tighten the javadoc claim that DefaultEndPoints "cache their resolved address and never re-resolve" -- true for a peer node's already-resolved physical IP, but a node whose EndPoint originated from an unresolved hostname does re-resolve via EndPoint.resolveAll() on every connect() call, independent of this flag.
…rIT (DRIVER-201) advanced.resolve-contact-points is now a documented no-op (contact points are always kept unresolved and expanded via EndPoint.resolveAll() at connection time). Remove the now-dead .withBoolean(TypedDriverOption.RESOLVE_CONTACT_POINTS..., false) line from all four MockResolverIT test methods that set it, so a future reader isn't misled into thinking it's load-bearing.
…dress (DRIVER-201) Addresses dkropachev's second review round. Five of the six comments trace back to two root causes, fixed here together because they meet in ChannelFactory: once it performs the address expansion itself, it also knows which concrete address a connection landed on. Resolve through Netty's AddressResolverGroup -------------------------------------------- DefaultEndPoint.resolveAll() no longer calls InetAddress.getAllByName(); it returns its address -- resolved or not -- as a single candidate. ChannelFactory expands unresolved candidates through the bootstrap's AddressResolverGroup, so a custom resolver installed via NettyOptions.afterBootstrapInitialized() is honoured again. That is the resolver an unresolved address already reached when it was handed straight to Bootstrap.connect(), so resolving anywhere else silently bypassed the user's configuration. Mirrors Bootstrap#doResolveAndConnect0: candidates the resolver does not support (LocalAddress) or that are already resolved (metadata nodes, whose endpoints hold addresses from the peers rows) pass through untouched, and a null group -- Bootstrap.disableResolver() -- is respected. A candidate that fails to resolve is skipped rather than failing the whole attempt; only an all-candidates failure fails the connect. The bootstrap is now built once per connect() instead of once per candidate, since it is the only handle on the resolver group; each attempt uses a clone() with its own handler. As a side effect the afterBootstrapInitialized() hook runs once per logical connection rather than once per address attempt. With Netty's default resolver the lookup blocks the I/O event loop it runs on, because DefaultNameResolver performs the JDK lookup inline. That is the pre-existing behaviour of handing an unresolved address to Bootstrap.connect(); the admin event loop -- the one control-connection reconnects run on, and the reason resolution was made async in the first place -- is still never blocked. Deployments needing non-blocking resolution can install DnsAddressResolverGroup and now have it take effect. Pin the connected address onto the channel ------------------------------------------ New internal PinnableEndPoint: a copy of an endpoint bound to one address. DefaultEndPoint, SniEndPoint and ClientRoutesEndPoint implement it with a nullable pinnedAddress excluded from equals/hashCode/asMetricPrefix, so a pinned copy denotes the same node and metric names do not change with the IP a connection happened to use. Equality stays symmetric, which a delegating wrapper could not offer -- endpoints are set and map keys. ChannelFactory hands the pinned copy to the channel initializer and the DriverChannel. Three consequences: - Node identity: once a node is known by host id it keeps reconnecting to the IP it was identified at. Previously DefaultTopologyMonitor#buildNodeEndPoint could store a shared multi-address endpoint for system.local, and since ControlConnection skips identity re-resolution for nodes that already have a host id, a later reconnect could reach a different node while still being treated as the original. - SniSslEngineFactory#newSslEngine() runs inside Netty's channel initializer. resolve() is now a field read there instead of a blocking getAllByName() on an event loop, and it returns the very proxy IP the channel is connected to. - GSSAPI: the authenticator receives a resolved endpoint, so getAddress().getCanonicalHostName() no longer NPEs on a contact point that is kept unresolved. A null-safe fallback to getHostString() is added anyway, for third-party endpoints that cannot be pinned. Endpoints that do not implement PinnableEndPoint are passed through unchanged, so third-party implementations behave exactly as before. Also in this change ------------------- - ClientRoutesEndPoint.resolveAll() runs topologyMonitor.resolve() on the supplied executor instead of the caller path -- it can reach InetAddress.getByName() -- and delegates to fallbackEndPoint.resolveAll() when there is no route, rather than flattening it to resolve(). - The resolver thread pool follows advanced.netty.daemon like every other driver thread, instead of hardcoding daemon threads. close() is what lets the JVM exit under the default non-daemon setting; its javadoc no longer claims otherwise. - Docs updated where they described expansion as happening inside the endpoint via JVM DNS: reference.conf, the upgrade guide, SessionBuilder and EndPoint.resolveAll()'s contract, which now states that returning a hostname is expected. The client-routes manual no longer says resolution blocks Netty I/O threads. Tests: DefaultEndPointTest covers the no-lookup contract and pinning identity in both directions; ChannelFactoryNettyResolverTest asserts a custom resolver is consulted, that all the addresses it returns are tried, that already-resolved candidates are left alone and that disableResolver() is respected; ChannelFactoryPinnedEndPointTest asserts the channel carries the address that connected while still equalling the original, and that non-pinnable endpoints are untouched; SniEndPointTest and ClientRoutesEndPointTest cover pinning and the executor hop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR's first commit removes OptionalLocalDcHelper#checkLocalDatacenterCompatibility, but nothing tested that removal. CUSTOMER-588 is the bug it caused. A contact point given as a hostname is represented, before the control connection resolves it, by an ephemeral placeholder Node built by MetadataManager#addContactPoints via DefaultNode#newContactPoint. Its datacenter is always null and never populated -- real topology is attached to a different Node object matched by hostId (see MetadataManager#registerNode). The removed check compared the configured local DC against those placeholders, so it warned unconditionally whenever a local DC was configured, no matter where the contact points actually were: You specified <dc> as the local DC, but some contact points are from a different DC: Node(endPoint=..., hostId=null, hashCode=...)=null The new test builds a real placeholder Node the same way production does, and a resolved node that genuinely is in the configured local DC, then asserts no warning is logged. It asserts on the absence of any WARN rather than of one particular message, so a regression that reintroduces the false positive under different wording is still caught; should_warn_if_configured_dc_matches_no_node is the positive control for the same appender, so a silent capture failure cannot make it pass by accident. The retained "configured local DC does not match any node's datacenter" check, which inspects the resolved node map, is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f6e2f5f to
603fa03
Compare
|
|
||
| /** | ||
| * The address this endpoint has been {@linkplain #pinTo(SocketAddress) pinned} to, or {@code | ||
| * null} if it is not pinned. Deliberately excluded from {@link #equals}, {@link #hashCode} and |
There was a problem hiding this comment.
Because the pin is excluded from equality, DefaultNode.setEndPoint() ignores a refreshed endpoint pinned to B when the stored endpoint is pinned to A. After contact-point fallback recovers through B, pools keep reconnecting to dead A. Preserve identity equality, but replace changed pins during refresh.
There was a problem hiding this comment.
Fixed in 5b79b630b6: DefaultNode.setEndPoint() now adopts the newest instance even when it compares equal, precisely so a refreshed pin replaces the stored one; only a genuine address change rebuilds the metric updater, since asMetricPrefix() is pin-independent. Identity equality is unchanged.
| || resolvedAddress.equals(this.pinnedAddress)) { | ||
| return this; | ||
| } | ||
| return new DefaultEndPoint(address, (InetSocketAddress) resolvedAddress); |
There was a problem hiding this comment.
Preserve the original DNS name when pinning the selected IP. A custom Netty resolver may return an InetSocketAddress built from a raw InetAddress; storing it verbatim makes SSL validate the IP or PTR instead of the configured DNS SAN.
There was a problem hiding this comment.
Fixed in ea8e1e2328, centrally in ChannelFactory rather than per endpoint: the queried name is re-attached to every expanded candidate before pinTo() ever sees it. f1abf32287 (today) takes it further — the queried name now wins over a resolver-supplied CNAME label too.
| public EndPoint pinTo(@NonNull SocketAddress resolvedAddress) { | ||
| Objects.requireNonNull(resolvedAddress, "resolvedAddress can't be null"); | ||
| if (!(resolvedAddress instanceof InetSocketAddress) | ||
| || resolvedAddress.equals(this.pinnedAddress)) { |
There was a problem hiding this comment.
Also return this when resolvedAddress.equals(address). Already-resolved contact points otherwise get a redundant pin, render as /A:9042(/A:9042), and break the five endpoint assertions in ZeroTokenNodesIT.
There was a problem hiding this comment.
Fixed in b83a8ae8a4: pinTo() returns this when the requested address is the one it already holds, which restored the ZeroTokenNodesIT assertions and the /A:9042(/A:9042) rendering. 00e2785b4c then aligned that shortcut across the other endpoint implementations.
| @Override | ||
| public InetSocketAddress resolve() { | ||
| return address; | ||
| return pinnedAddress != null ? pinnedAddress : address; |
There was a problem hiding this comment.
This pinned contract makes MockResolverIT.should_connect_with_mocked_hostname fail its unchanged isUnresolved() assertion. Update the test to expect the selected resolved IP and verify the original hostname separately.
There was a problem hiding this comment.
Agreed, and the assertion was the right thing to change: b83a8ae8a4 updates it to expect the pinned IP the control connection landed on, plus a check that asMetricPrefix() still keys off the hostname. Verified today against live Scylla — all three MockResolverIT tests pass.
| // matches the channel class, since DnsAddressResolverGroup registers a datagram channel on it. | ||
| // An I/O event loop is also what Netty itself uses here: Bootstrap resolves on the connecting | ||
| // channel's own event loop. | ||
| EventExecutor eventExecutor = context.getNettyOptions().ioEventLoopGroup().next(); |
There was a problem hiding this comment.
This advances the shared I/O chooser, and Bootstrap.connect() advances it again when registering the channel. With the default even-sized group, normal connections use only half the I/O loops. Resolve on the channel's selected loop or avoid the extra next().
There was a problem hiding this comment.
Fixed in 73f9342019: the event loop is now taken once per connect() and shared by resolution and the channel (each attempt uses clone(eventLoop)), so the group chooser advances exactly once per logical connect instead of twice.
| new LinkedBlockingQueue<>(), | ||
| runnable -> { | ||
| Thread thread = | ||
| new Thread( |
There was a problem hiding this comment.
Create these workers through BlockingOperation.SafeThreadFactory, then apply the name and daemon settings. Plain threads bypass the driver's synchronous-call guard, so custom endpoint resolution can deadlock instead of being rejected.
There was a problem hiding this comment.
Moot as of 5b79b630b6: resolution moved onto the channel event loop and the driver-created resolver executor is gone, so there are no driver threads left here to route through SafeThreadFactory.
| * same contract as {@link NettyOptions#onClose()}. | ||
| */ | ||
| public void close() { | ||
| resolverExecutor.shutdownNow(); |
There was a problem hiding this comment.
Make resolver termination part of the bounded asynchronous close sequence. shutdownNow() only interrupts workers and returns; blocked non-daemon resolver code can outlive completed session close and keep the JVM running.
There was a problem hiding this comment.
Same root as the thread above — 5b79b630b6 removed the resolver executor entirely, so there is nothing left to terminate as part of the close sequence.
| result.completeExceptionally(t); | ||
| return; | ||
| } | ||
| expandCandidate(resolver, candidates, 0, new ArrayList<>(), null, result); |
There was a problem hiding this comment.
Wrap the entire resolver call path in try/catch. If a custom resolver throws synchronously from isSupported, isResolved, or resolveAll, this event-loop task exits and result never completes, hanging initialization or reconnection.
There was a problem hiding this comment.
Fixed in a0328a7e87: there are now blanket catches around the event-loop task body, the resolveAll() listener body and the execute() call itself, so a synchronous throw from a custom resolver fails the connect future instead of leaving it pending forever.
| * miss fallback IPs when the first one is unreachable. {@code resolveAll(Executor)} returns | ||
| * the full set, resolved asynchronously off the calling thread. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Please do not deprecate a method that every EndPoint implementation must still override. External implementations compiled with -Xlint:deprecation -Werror now fail solely because their mandatory override is deprecated.
There was a problem hiding this comment.
Agreed — the deprecation is gone as of 5b79b630b6, along with the resolveAll() API addition it came with. Resolution is a connection-layer concern now, so resolve() is back to being the plain undeprecated contract every implementation overrides.
| initializer(endPoint, currentVersion, options, nodeMetricUpdater, resultFuture)); | ||
|
|
||
| .option(ChannelOption.ALLOCATOR, nettyOptions.allocator()); | ||
| nettyOptions.afterBootstrapInitialized(bootstrap); |
There was a problem hiding this comment.
Preserve the previous hook ordering: afterBootstrapInitialized() used to receive a bootstrap with the driver initializer already installed. It now sees no handler, and the later clone overwrites any handler it installs, breaking existing hooks that inspect, validate, or wrap it.
There was a problem hiding this comment.
This is the one I answered by design rather than by code (426d9508c6): each candidate attempt takes its own clone(), so a handler installed on the base bootstrap could not survive anyway. The contract is now documented and a stray handler warns once. Happy to revisit if you would rather the hook ran per attempt.
Both CI failures introduced by 648c824 trace to the pinning half of it. ZeroTokenNodesIT (5 tests, Scylla serial jobs) ---------------------------------------------- ChannelFactory pins every connection's endpoint to the address it reached, including already-resolved ones -- and for those the "address it reached" is the address the endpoint already holds, since a resolved candidate passes through the resolver untouched. The resulting copy was indistinguishable from the original except in toString(), which grew a redundant suffix: /127.0.13.3:9042(/127.0.13.3:9042) DefaultEndPoint.pinTo() now returns this when the requested address is the one it already holds. For this class that is a genuine no-op -- resolve(), resolveAll() and toString() all keep yielding exactly what they did -- so it also spares an allocation on every connect to a resolved endpoint, which is every node discovered from the peers rows. Deliberately not applied to SniEndPoint or ClientRoutesEndPoint: their unpinned resolve() resolves lazily (getAllByName() on the proxy hostname, ClientRoutesTopologyMonitor.resolveAddress()), so for them a pinned copy is meaningful even when it matches the stored address -- that is what took the blocking lookup off the event loop in SniSslEngineFactory#newSslEngine(). MockResolverIT.should_connect_with_mocked_hostname (isolated jobs) ----------------------------------------------------------------- This one is the intended behaviour change, so the assertion is updated rather than the code. The control node's endpoint is now the pinned copy (DefaultTopologyMonitor#buildNodeEndPoint stores the channel's endpoint), so resolve() yields the IP the control connection landed on instead of the unresolved hostname. The test now asserts that, plus that asMetricPrefix() still keys off the hostname -- the pinned copy denotes the same node. The guarantee the old assertion protected is unaffected: contact points stay unresolved and are re-added to the reconnection plan, which is what lets a replaced cluster be picked up. replace_cluster_test() covers that and passes. The residual trade-off is deliberate: a node identified through a hostname keeps reconnecting to the pinned IP, so if that IP changes under a stable host id, recovery goes through the contact points rather than through the node itself. That is the cost of the stable node identity requested in review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed
|
…nt API addition (DRIVER-201) `EndPoint.resolveAll(Executor)` was added for two reasons: return every IP a hostname maps to, and do it asynchronously so the admin event loop never blocks on DNS. `resolve()` was deprecated on that basis. Moving resolution into ChannelFactory (648c824) invalidated both: - Multiplicity is produced by ChannelFactory through Netty's resolver, not by resolveAll(). DefaultEndPoint.resolveAll() had become literally `completedFuture(new SocketAddress[]{resolve()})` -- one element, no lookup, the executor never touched. - Asynchrony was only still needed because SniEndPoint and ClientRoutesEndPoint chose to resolve internally. Both can simply stop, which is what this does. Meanwhile resolve() -- deprecated for "missing fallback IPs" -- is what the driver itself calls in eight places, and pinning had made it the precise accessor for "the address this channel is on". The deprecation had become advice against the driver's own design. Neither resolveAll() nor the @deprecated exists on scylla-4.x: both were new in this PR, so there is nothing to keep compatible with. Every EndPoint implementation in the repo is internal. The principle ------------- An EndPoint describes *where* a node is. It never performs name resolution. Resolution happens once, in ChannelFactory, through Netty's AddressResolverGroup. - EndPoint: resolveAll() removed, resolve() un-deprecated. Its contract now states that returning an unresolved address is how multi-address support works, and that implementations must neither resolve names nor block. - SniEndPoint: no more getAllByName(), no rotation counters, no IP comparator. resolve() returns the pinned proxy IP, else the configured proxy address -- already unresolved, as CloudConfigFactory builds it. Netty expands it, so SNI gains multi-proxy-IP fallback *and* custom-resolver support, neither of which it had. - ClientRoutesEndPoint / ClientRoutesTopologyMonitor: the route hostname is returned unresolved from the in-memory cache instead of going through InetAddress.getByName(). resolve() is now a pure cache read; the protected resolveAddress() hook is gone with its only caller. - ChannelFactory: takes the single address from resolve() and expands it. The resolver thread pool is deleted outright -- nothing blocks any more -- along with its advanced.netty.daemon handling, close(), and the DefaultSession call. The round-robin SniEndPoint used to do moves here as rotate(), so it now applies to every endpoint type rather than only SNI. - PinnableEndPoint is kept as-is: internal, and the part of 648c824 that earns its place. Against dkropachev's review round, this leaves three comments fixed as they were (GSSAPI NPE, node identity, blocking DNS in newSslEngine -- all by pinning), gives a better answer to two (the client-routes blocking is eliminated rather than offloaded; the custom resolver now reaches SNI and client routes too), and makes one moot (no resolver threads left to honour advanced.netty.daemon). Also fixed here --------------- - DefaultNode.setEndPoint() gated its whole body on !equals(), and equals() ignores pinnedAddress by contract -- so a stale pin could never be replaced and the control node stayed frozen on the first address it connected to, even after the control connection had moved and told us about it. It now always adopts the newest instance, with only the metric-updater rebuild still gated on a genuine address change (asMetricPrefix() is pin-independent, so a pin-only change must not churn metrics). - TopologyMonitor.reresolvesNodeAddresses() claimed the connected node's endpoint re-resolves on every connection attempt. Pinning made that false; the javadoc now says the endpoint is bound to the address its control connection reached, and that recovery depends on this flag being false. - Eight @SuppressWarnings("deprecation") annotations that existed only for the resolve() deprecation are removed. Behaviour worth calling out: resolve() on an *unpinned* SniEndPoint or ClientRoutesEndPoint may now return an unresolved address where it previously returned a resolved one. Every in-tree caller holds a channel endpoint, which is always pinned (SniSslEngineFactory, DefaultTopologyMonitor#savePort and #getBroadcastRpcAddress, GssApiAuthenticator); InsightsClient reads node endpoints, which are resolved for peers and pinned for the control node. A third-party EndPoint that blocks inside resolve() will block the admin loop again, exactly as in the released driver -- this gives up an improvement the previous revision of this PR briefly offered, in exchange for no public API change at all. Tests: ChannelFactoryAsyncResolveTest and ChannelFactoryResolveAllGuardTest are deleted (they guarded contracts that no longer exist); ChannelFactoryMultiAddressTest now drives expansion through a resolver and covers rotation plus a throwing resolve(); the resolver stub is extracted to TestAddressResolverGroup and shared; the endpoint tests assert that no endpoint performs a lookup; DefaultNodeTest covers pin adoption and metric non-churn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed Why
Meanwhile Neither The principle
Two things made that reachable without losing behaviour:
Net effect: −390 lines (484 added, 874 removed), and Against your review round
Two other fixes in the same commit
Also removed eight Behaviour worth flagging
And the cost, stated plainly: a third-party Verified with |
…IVER-201) Fallout from 5b79b63: a client route is now handed to the connection layer unresolved, so `endPoint.resolve()` yields an unresolved InetSocketAddress for a proxied node and `getAddress()` is null. Three assertions dereferenced it and NPE'd on the Scylla LATEST/LTS-LATEST isolated jobs (the two backends that have system.client_routes): ClientRoutesIT.classifyNodes:209 ClientRoutesIT.collectHostIds:323 ClientRoutesIT.should_refresh_routes_after_table_update:543 All three compare against IP literals (NLB_ADDRESS, the ccm node address), so getHostString() is the right accessor: it returns the literal for a resolved address and the hostname for an unresolved one, and is never null. The refresh-after-update assertion also now states outright that the route comes back unresolved, so the contract is pinned rather than incidental. Driver behaviour is unaffected -- this is test-side only. MockResolverIT (3/3, including replace_cluster_test and the dead-first-DNS-entry case) passed in the same run, as did every Cassandra isolated/serial job and every Scylla serial job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed A client route is now handed to the connection layer unresolved, so
All three compare against IP literals ( This is exactly the consequence I flagged in the previous comment; I had checked every production caller but not this IT's runtime assumptions, only its compile error. Driver behaviour is unaffected. What the same run established, so this isn't read as a behavioural break:
I also checked the other ITs that call The remaining red check, |
…p callbacks (DRIVER-201) ChannelFactory.connect() had no timeout of its own, yet several of its async seams could die without completing the caller's future, hanging control-connection init or a pool reconnect forever: - resolveCandidates() only guarded getResolver(). A custom resolver throwing synchronously from isSupported()/isResolved()/resolveAll(), or a throw from the resolveAll listener body, killed the event-loop task with the future still pending (Netty only logs those). - eventExecutor.execute() itself throws RejectedExecutionException while the group shuts down, and escaped synchronously out of connect(), which never used to throw. - tryNextCandidate() runs in CompletionStage continuations that swallow throwables; a custom PinnableEndPoint.pinTo() throwing was lost. - connectToAddress()'s connect listener contains the downgrade recursion, the version-registry lookup and the cloud config override, all inside a Netty listener that swallows throwables. - A third-party EndPoint.resolve() returning null (contractually forbidden) NPE'd inside the event-loop task instead of failing fast; before multi-address support this failed synchronously in Bootstrap.connect(null). Establish the invariant that every path completes the future: blanket try/catch around the resolver task, the resolveAll listener, the execute() dispatch, tryNextCandidate() and its whenComplete continuation, connectToAddress()'s synchronous section and its connect listener, plus a fail-fast null check after EndPoint.resolve(). Double completion is harmless: completeExceptionally() on a completed future is a no-op, which the existing initializer error path already relies on. Tests cover each seam: a resolver whose every method throws, a null resolve(), a shut-down event loop group (rejected dispatch), a throwing pinTo(), and a version registry that throws inside the connect listener. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…IVER-201) The channel's pinned endpoint is built from the candidate address the resolver returned, and it is what DefaultSslEngineFactory and SniSslEngineFactory derive the SSL peer host from, inside the channel initializer. The JDK and Netty-DNS resolvers attach the queried name to the InetAddresses they return, but a custom resolver may build its results from raw address bytes. With such a nameless address: - InetSocketAddress#getHostName() triggers a blocking reverse-DNS lookup on the Netty event loop during SSL engine creation -- the very thing pinning was introduced to eliminate; and - TLS hostname validation checks the certificate against the IP or the PTR record instead of the name the user configured, failing (or worse, passing against a name the operator never chose). Re-attach the queried hostname centrally in ChannelFactory, right after expansion, so every endpoint type is covered in one place and pinTo() stores an address that already carries the right name. InetAddress.getByAddress(host, bytes) performs no lookup; the TCP connect target, address equality and rotation determinism are all unchanged. A candidate that already carries a real name (e.g. a CNAME target) is respected, and scoped IPv6 addresses are left alone since a rebuild would drop the scope id. Tests cover the re-attach (asserting getHostName() itself, which proves no reverse lookup happens), the resolver-name-wins case, non-Inet and already-resolved pass-through, bare IPv6, and scoped IPv6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…DRIVER-201) tryNextCandidate()'s javadoc promised that protocol-version negotiation exhaustion does not advance to the next candidate, but the code advanced on any error. A protocol-version rejection -- negotiation exhausting every downgrade, or the server refusing a forced version -- is a property of the node, not of the address the connection happened to use, so replaying the whole negotiation ladder against every remaining IP of the same name bought nothing and stretched the worst-case failure time from the documented N x connect-timeout to N x versions x connect-timeout. Make UnsupportedProtocolVersionException terminal in the candidate loop, matching both the javadoc and the pre-multi-address behaviour of a single-address connect. The javadoc now also spells out the corner this deliberately does not rescue (a heterogeneous rolling upgrade where IPs behind one name support different protocol versions) and that TCP/init/auth failures still advance, since those may well be address-specific. The new test expands a name to the same live server twice (sidestepping rotation nondeterminism), exhausts negotiation on the first candidate, and asserts the second is never attempted plus that the propagated UnsupportedProtocolVersionException carries no suppressed connect errors. The no-second-attempt check uses a new non-failing tryReadOutboundFrame() base helper and runs before the future assertion, so a regression drains the stray frame and fails cleanly instead of deadlocking the server-side exchanger in tearDown(). Also hoist the installResolver() helper, duplicated across two test classes and inlined in a third, into ChannelFactoryTestBase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t (DRIVER-201) resolveCandidates() took one event loop from the I/O group for name resolution, and Bootstrap.connect() then advanced the group's chooser again when registering the channel. Every unresolved-address connect -- which is all cloud/SNI pool connections, client routes, and contact points -- therefore advanced the round-robin chooser by exactly two, and with the default power-of-two chooser that parks every channel on loops of a single parity: half the I/O threads carry all the traffic. Pick the event loop once per logical connect, run resolution on it, and bind the per-attempt bootstrap clones to it with clone(EventLoop): the chooser now advances exactly once per connect on both the resolved and unresolved paths, and resolution runs on the connecting channel's own loop -- which is precisely what Netty's Bootstrap does with an unresolved address. The base bootstrap keeps the full group, so the afterBootstrapInitialized() hook observes the same group as before. The new test registers which executor the resolver was created for and asserts the connected channel's event loop is that same object, using a two-thread group: with the base's single-thread group the assertion would be vacuous, while with two threads the old code deterministically split resolution and registration across different loops. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rotation offset was one global counter shared by every name the driver expands. Names whose expansions interleave in lockstep -- for example two hostname contact points expanded in sequence on every control-connection reconnection round -- each only ever saw one offset parity, pinning every name with an even record count to a fixed starting address and defeating the rotation entirely. This is the same failure mode that once collapsed SniEndPoint's rotation when SSL engine setup shared its counter (fixed then by splitting the counters), now across names instead of across methods. Track one counter per name, keyed by the queried address's lowercased host string, with a single fallback counter for the rare non-name-based original. The map is never evicted; its keys are the distinct names the driver ever expands (contact points, the SNI proxy name, client-route hostnames), each holding one AtomicInteger, so growth is bounded by configuration and topology. The single-address short-circuit now also documents (and the test asserts) that no counter is created or advanced for it. The new independence test interleaves two fresh names and asserts each rotates on its own and neither perturbs the other -- it fails with a shared global counter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ay handlers (DRIVER-201) Moving name resolution into ChannelFactory changed the hook's contract in two ways that were previously implicit: - it now runs once per logical connection to a node, instead of once per attempt (which included protocol-version downgrade retries) -- the per-address attempts and downgrade retries share the bootstrap through clone(EventLoop); - it receives the bootstrap before the driver installs its channel handler, and a handler set by the hook is replaced by the driver's own on each per-attempt copy. Previously the hook ran after .handler(...), so replacing the driver's handler was technically possible, though never a supported extension point. Spell both out in the NettyOptions.afterBootstrapInitialized() javadoc (options, attributes and Bootstrap.resolver() are what the hook is for; pipeline customization belongs in afterChannelInitialized()), log a one-time warning when the hook is detected installing a handler -- following the LOGGED_ORPHAN_WARNING pattern -- and document the change in the upgrade guide. The new test installs a dummy handler from the hook and asserts the connection still completes its protocol handshake, proving the driver's handler is the one that ends up on the channel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…RIVER-201) The stub and its comment referred to ChannelFactory's name-resolver thread pool, which was removed when resolution moved to Netty's AddressResolverGroup; the factory no longer reads advanced.netty.daemon at all (only DefaultNettyOptions does, and these tests mock NettyOptions). Harmless today only because the base class uses lenient initMocks(), but a misleading breadcrumb for the next reader. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ementations (DRIVER-201) ClientRoutesEndPoint accepted and stored any SocketAddress as its pin, while DefaultEndPoint and SniEndPoint reject non-InetSocketAddress pins; downstream readers of a pinned endpoint's resolve() (the GSSAPI authenticator's cast, DefaultTopologyMonitor's instanceof guards) expect Inet addresses. Tighten the field and guard to match the siblings: a non-Inet address skips pinning instead of being stored. SniEndPoint gains DefaultEndPoint's remaining shortcut: pinning to the very address the endpoint already holds returns the same instance, sparing the copy and its redundant "proxy(proxy)" toString suffix. Only reachable when the proxy address was supplied already resolved -- Cloud supplies a hostname, for which a resolved pin never compares equal. (The earlier reason for skipping this shortcut -- that SniEndPoint's unpinned resolve() used to look the proxy up lazily, making even a same-address pin meaningful -- no longer holds now that resolve() is a field read.) The stale toString() comment claiming channels always carry a pinned copy is updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| try { | ||
| if (error == null) { | ||
| resultFuture.complete(channel); | ||
| } else if (!(error instanceof UnsupportedProtocolVersionException) |
There was a problem hiding this comment.
A contact-point hostname can expand to different nodes. If the first candidate exhausts negotiation, or rejects a forced version, this condition stops before trying a later candidate that may support the protocol. Before this PR, resolve-contact-points=true represented each resolved address as a separate node and the control connection advanced after such a failure. Please continue to the next candidate when node identity is not established; keep this terminal shortcut only for known nodes.
There was a problem hiding this comment.
You are right, and it was a regression: ControlConnection advances its query plan on any error, so with resolve-contact-points=true each resolved address got that advance for free. Fixed in 1b3aea1812 — the shortcut is now gated on Node.getHostId() != null, so an unidentified contact point keeps trying its remaining addresses.
| || candidateIp == null | ||
| // The candidate already carries a real name: respect the resolver's choice. (For a | ||
| // nameless address, getHostString() falls back to the IP literal; no lookup either way.) | ||
| || !candidateInet.getHostString().equals(candidateIp.getHostAddress()) |
There was a problem hiding this comment.
Always preserve the queried hostname here, even when the resolver supplies a canonical or CNAME label. The candidate is pinned onto the channel endpoint and then used by the SSL factories, so retaining the resolver label makes endpoint identification validate that name instead of the hostname configured by the user. Before this PR the initializer kept the original endpoint while Netty resolved only the TCP destination.
There was a problem hiding this comment.
Agreed — a resolver label ends up being what TLS verification checks the certificate against. Fixed in f1abf32287: the queried name now always wins, and the bail-out became "the candidate already carries it" (the JDK/Netty-DNS case). Only scoped IPv6 is still passed through, since a rebuild drops the scope id.
| if (original instanceof InetSocketAddress) { | ||
| // DNS names are case-insensitive; normalize so the same name shares one counter. | ||
| String name = ((InetSocketAddress) original).getHostString().toLowerCase(Locale.ROOT); | ||
| return ROTATION_OFFSETS.computeIfAbsent(name, k -> new AtomicInteger()); |
There was a problem hiding this comment.
This static map retains every multi-address hostname for the lifetime of the JVM. Client-route hostnames can change during refreshes, topology can churn, and successive sessions can use unrelated names, so historical entries are not bounded by the current configuration or topology. Please scope the counters to a session or use a bounded/evicting cache.
There was a problem hiding this comment.
Fixed in 0f6d91af9f, both ways: the counters moved onto ChannelFactory (so per session, not per JVM) and sit behind a 256-entry evicting cache, since client-route hostnames can churn within one long-lived session too. An evicted counter only costs that name a rotation restart.
…n rejection (DRIVER-201) tryNextCandidate() treated every UnsupportedProtocolVersionException as terminal. That is right for a node we have already identified -- all of its addresses are that same node, so replaying the negotiation ladder against each one buys nothing -- but wrong for a contact point: the addresses one name expands to may belong to different nodes, and a rejection by the first says nothing about the rest. It was also a regression. With advanced.resolve-contact-points = true each resolved address used to be a separate Node, and ControlConnection.SingleThreaded.connect() advances to the next node in its query plan on any error, this one included. Collapsing a name into a single Node moved that responsibility into the candidate loop, so the loop has to honour it. Thread node identity down from the connect(Node, ...) entry points: Node.getHostId() is null only for an initial contact point, until host ids have been read from system.local and system.peers for the first time, which is exactly the "we do not know which node this is" case. The shortcut now applies only to identified nodes. The @VisibleForTesting connect(EndPoint, ...) overload keeps its signature and passes "unidentified", since a bare endpoint carries no host id either; a new overload takes the flag explicitly. The existing terminal-shortcut test now drives the identified path, and a mirror test covers the unidentified one: the second candidate is tried, replays the ladder from the top, and the propagated UnsupportedProtocolVersionException carries the first candidate's failure as a suppressed exception. The negotiation-ladder mocking and the server-side exchange they share are now helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ses (DRIVER-201) reattachHostname() only re-attached the queried name to a candidate that carried no name of its own, deferring to a resolver that labelled its results with a canonical or CNAME name. But that label is not cosmetic: the candidate is pinned onto the channel endpoint, and DefaultSslEngineFactory / SniSslEngineFactory derive the SSL peer host from it inside the channel initializer. So the resolver's label became the name TLS hostname verification checked the server certificate against -- a name the operator never configured. Before multi-address support the initializer kept the original endpoint and Netty resolved only the TCP destination, so the configured name was always the one validated. Make the queried name win unconditionally. The bail-out is now "the candidate already carries the queried name", which is the common case (the JDK and Netty-DNS resolvers attach it themselves) and keeps the no-op cheap; the scoped-IPv6 exception stays, since a rebuild would drop the scope id. Uniform names across an expansion also make rotate()'s toString() sort depend only on the IP and port, so ordering gets more deterministic, not less. The resolver-name test now asserts the queried name replaces the CNAME label, and a new test covers the already-has-the-name pass-through that used to be implied by it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m (DRIVER-201) The per-name rotation offsets lived in a static map, so every multi-address name the driver ever expanded stayed in it for the lifetime of the JVM. Its keys are not bounded by the current configuration or topology: client routes can hand out different hostnames on every refresh, topology churns, and successive sessions in the same JVM can use entirely unrelated names. Move the counters onto the ChannelFactory -- one per session, so they go away with it -- and bound them with an evicting cache on top, since the churn within a single long-lived session is unbounded too. Spreading connections only ever matters among the names a session is currently using, so an evicted counter costs that name nothing but a rotation restart. The cache uses the shaded-Guava idiom already used for the codec and prepared-statement caches. rotate() and rotationOffsetFor() become instance methods; the rotation tests now go through a factory, which also makes them self-contained -- they no longer need names unique across the whole class to avoid inheriting another test's offset. Two new tests cover what the change is for: separate factories do not share offsets, and the tracked-name count stays bounded when a session churns through many names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (4)
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java:709
- This public option documentation says expansion happens at query-plan time, but the implementation deliberately keeps one unresolved node in the plan and expands it in
ChannelFactoryat connection time. Correct the wording to avoid contradictingSessionBuilder,reference.conf, and the new connection path.
* <p>This is also the driver's DNS re-resolution path: contact points are expanded to their
* current DNS IPs at query-plan time, whereas metadata nodes hold an already-resolved endpoint
* that is never re-resolved. Keeping this enabled lets control-connection reconnects re-resolve
core/src/main/java/com/datastax/oss/driver/api/core/metadata/EndPoint.java:61
- The PR description still documents a different public API and implementation: it says
resolve()is deprecated, introducesEndPoint.resolveAll(), and performs endpoint-level JVM DNS resolution, while this diff keepsresolve()unchanged and expands addresses through Netty inChannelFactory. Update the description and test summary to match the implementation so reviewers and release notes do not advertise an API that is absent.
@NonNull
SocketAddress resolve();
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java:431
- This early return does not mirror Netty for custom resolvers:
Bootstrap#doResolveAndConnect0asks the configured resolver whether an address is resolved, even when it is a resolvedInetSocketAddress. A customAddressResolvercan deliberately returnfalseand remap that address, but this branch now bypasses it and changes the destination compared with the previousBootstrap.connect()path. Remove the hard-coded shortcut and let the existingresolver.isSupported()/isResolved()check below decide; the default resolver will still pass normal resolved addresses through.
if (isResolved(address)) {
return CompletableFuture.completedFuture(Collections.singletonList(address));
core/src/main/java/com/datastax/oss/driver/internal/core/channel/ChannelFactory.java:553
- Skipping hostname reattachment for a scoped IPv6 candidate makes the pinned endpoint expose the raw link-local IP (or its PTR name).
DefaultSslEngineFactoryandSniSslEngineFactorythen use that value as the TLS peer host, so a custom resolver returning a nameless scoped address can connect at TCP level but fail hostname verification against the originally queried hostname. Preserve the scope while attaching the hostname using theInet6Address.getByAddressoverload that accepts the scope interface or ID, and update the scoped-IPv6 test accordingly.
// Rebuilding a scoped IPv6 address would silently drop its scope.
|| (candidateIp instanceof Inet6Address
&& (((Inet6Address) candidateIp).getScopeId() != 0
|| ((Inet6Address) candidateIp).getScopedInterface() != null))) {
Problem
DRIVER-201: when a contact point or a cluster node is given as a hostname that maps to multiple IPs (e.g. a DNS round-robin / dynamic-DNS entry), the driver only ever tried the first address — at initial contact, at connection time, and on control-connection reconnect. If that first IP was unreachable the driver raised
AllNodesFailedExceptioneven though the hostname also resolved to healthy IPs.This PR fixes DRIVER-201 end-to-end: every such hostname is now resolved to all its addresses and each is tried in turn, at both the contact-point and the general connection layer.
Changes
EndPointinterfaceresolve()is now@Deprecated.resolveAll()default method returnsSocketAddress[]. The default implementation wrapsresolve()in a single-element array, so existing third-party implementations keep working (no new abstract method → source/binary compatible).DefaultEndPointresolveAll(): for unresolved addresses callsInetAddress.getAllByName()and returns oneInetSocketAddressper IP (built from the resolvedInetAddressso the original hostname is retained for TLS peer host / SNI / hostname verification). Falls back to a single-element array (the unresolved address) if DNS fails, so the connect attempt surfaces a descriptive error rather than an empty array.SniEndPointresolveAll(): re-resolves the proxy hostname on each call, sorts all A-records by IP, and returns all records so a single connection attempt can fall back across every proxy IP. The candidate order is rotated each call using the same round-robinOFFSETcounter asresolve(), so healthy connections stay spread across proxy IPs instead of always starting at index 0. (dkropachev'sCHANGES_REQUESTEDfix.)ClientRoutesEndPointresolveAll(): wraps the single topology-monitor-resolved address in a one-element array (single-address by design).ChannelFactoryconnect()now callsendPoint.resolveAll()instead ofendPoint.resolve(), and guards against anull/empty array from a customEndPointby failingresultFuture(instead of NPE/AIOOBE).tryNextCandidate()iterates the returned array; on per-address failure it logs and tries the next; only fails the overallresultFutureonce all candidates are exhausted.connectToAddress()scopes protocol-version negotiation (downgrade retries) to a single address.connect()now serially attempts every candidate, so the worst-case time to declare a node unreachable isN × connect-timeout. This is an intentional trade-off (failing on the first unreachable IP would prevent fallback) and is documented onEndPoint.resolveAll().Remove the interim query-plan resolution
Now that
ChannelFactory.connect() → resolveAll()handles multi-address fallback at connection time — for both control-connection init and pool connections — the earlier interim query-plan-time DNS expansion (the contact-point hostname expansion from the initial approach) is redundant and is removed:MetadataManager: dropsgetResolvedContactPoints()and its dedicated resolver executor, 3s timeout, and helpers. That method resolved contact-point hostnames on the admin event loop (offloaded to a bounded executor becauseInetAddress.getAllByName()blocks and the admin loop must never block). The event loop no longer blocks on DNS at all — the query plan now holds one unresolved node per contact point, andresolveAll()expands each to all its IPs at connect time.LoadBalancingPolicyWrapper/InsightsClient: revert togetContactPoints(). TheRUNNING-state reconnection fallback and theTopologyMonitor.reresolvesNodeAddresses()gate are preserved.reference.conf,SessionBuilder,DefaultEndPoint) updated to say resolution happens at connection time viaEndPoint.resolveAll().Control-connection reconnection query plan (folded from #889 review)
LoadBalancingPolicyWrapper.newControlReconnectionQueryPlan()now composes the contact-point fallback viaCompositeQueryPlan(regularPlan, new SimpleQueryPlan(contactNodes))instead of mutating the policy's plan. Built-inQueryPlans rejectadd()/addAll()(poll()is their only mutator), so the previousaddAll(...)threwUnsupportedOperationExceptionon every post-init control reconnect once the fallback defaulted on. The fallback is also kept when the live-node plan is empty, even for re-resolving topology monitors, so reconnection can still recover when there is nothing else to try. The wrapper tests now stub the policy plan with a realSimpleQueryPlan/QueryPlan.EMPTY(the earlier mutableLinkedListstub masked the crash), plus a new empty-plan + re-resolving-monitor case. (dkropachev's #889CHANGES_REQUESTEDfix.)OptionalLocalDcHelperRemoves the dead
checkLocalDatacenterCompatibility()check as part of this history cleanup. It warned when a contact point's datacenter differed from the configured local DC, but contact-point nodes never get a datacenter assigned during refresh, so the check compared againstnulland could never reflect a real mismatch. The separate "configured local DC matches no node" warning is retained. Unrelated to the DNS-resolution fix itself, called out here since it touches aprotectedextension point.Internal callers
Callers that legitimately need a single canonical address (
InsightsClient,DseGssApiAuthProviderBase,DefaultTopologyMonitor, the SNI / Default SSL engine factories) keep calling the deprecatedresolve()under a scoped@SuppressWarnings("deprecation").Tests
DefaultEndPointTest: already-resolved passthrough, unresolved hostname expansion, unresolvable hostname fallback.SniEndPointTest:resolveAll()happy path, unresolvable host exception,resolve()sanity check, and a rotation/completeness case asserting the full candidate set is returned every call and the starting candidate rotates when multiple IPs exist.ChannelFactoryResolveAllGuardTest: null array, empty array, andresolveAll()throwing all fail the connect future.LoadBalancingPolicyWrapperTest: realQueryPlanstubs; append-ordering, empty-plan, and re-resolving-monitor cases for the control-reconnection plan.ChannelFactorytests pass unchanged (LocalEndPointuses the default single-elementresolveAll()via the interface default).MetadataManagerTestcontact-point resolution/timeout unit tests (that behavior now lives inDefaultEndPointTest.resolveAlland theChannelFactorytests); adapted theLoadBalancingPolicyWrapper/InsightsClienttests togetContactPoints().Review follow-up (earlier rounds)
ChannelFactory: scoped the protocol-version negotiation history (attemptedVersions) to each candidate address individually, instead of sharing one list across every candidate — avoids a misleadingUnsupportedProtocolVersionExceptionmessage that could conflate negotiation attempts from two different IPs.ChannelFactory: boundedresolverExecutorto a fixed 16-thread pool5b79b630b6moved resolution onto the channel's own event loop (through Netty'sAddressResolverGroup) and removed the driver-created resolver executor altogether, so there is no pool left to size, no daemon-flag question, and nothing to terminate on close.LoadBalancingPolicyWrapper/TopologyMonitor: doc-only clarifications — a narrow, benign state-read race window innewControlReconnectionQueryPlan(), and a more precisereresolvesNodeAddresses()javadoc.MockResolverIT: removed the now-inertadvanced.resolve-contact-pointsconfig line from the tests that still set it.Review follow-up (2026-08-03)
Three revisions to the candidate loop, all on code the previous round introduced:
1b3aea1812). AnUnsupportedProtocolVersionExceptionstill ends the attempt for a node whose host id is known — all of its addresses are that same node — but an unidentified endpoint (a contact point, before host ids have been read) keeps trying its remaining addresses, since one name may expand to addresses of different nodes. This restores what collapsing a name into a singleNodewould otherwise have removed: withadvanced.resolve-contact-points = trueeach resolved address used to be a separateNode, andControlConnectionadvances its query plan on exactly this error.f1abf32287).reattachHostname()used to defer to a resolver that labelled its results with a canonical/CNAME name. That label reaches the pinned endpoint and is therefore whatDefaultSslEngineFactory/SniSslEngineFactorymake TLS hostname verification check the certificate against, so the configured name now always wins. Only scoped IPv6 is still passed through, since rebuilding it would drop the scope id. Uniform names across an expansion also makerotate()'s sort depend on the IP and port alone.0f6d91af9f). They moved off a static map onto theChannelFactory, behind a 256-entry evicting cache. The names that reach them — contact points, the SNI proxy name, client-route hostnames — are not bounded by the current configuration or topology: client routes can hand out different hostnames on every refresh. Spreading only matters among the names a session is currently using, so an evicted counter costs that name nothing but a rotation restart.Verified on JDK 11: full
coreunit suite (3844 tests) andMockResolverITagainst live ScyllaDB 2026.1.9.