Skip to content

Commit 791105c

Browse files
nikagraclaude
andcommitted
feat: fall back to contact points on control reconnect (DRIVER-201)
Default fallback-to-original-contact-points to true: once a control reconnection round has exhausted the live nodes, the unresolved contact points are appended, so Netty resolves their names again and a cluster that moved is found. Every metadata node, the control node included, otherwise holds an address resolved once. Monitors that re-resolve on their own opt out through TopologyMonitor.reresolvesNodeAddresses(), unless the live-node plan is empty. The control node's identity is applied to the channel at adoption, so every node refresh reads one endpoint. HeartbeatIT turns the fallback off; MockResolverIT gains the recovery case. Fixes #215 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5725116 commit 791105c

23 files changed

Lines changed: 935 additions & 140 deletions

File tree

core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -701,8 +701,21 @@ public enum DefaultDriverOption implements DriverOption {
701701
CONTROL_CONNECTION_AGREEMENT_WARN("advanced.control-connection.schema-agreement.warn-on-failure"),
702702

703703
/**
704-
* Whether to forcibly add original contact points held by MetadataManager to the reconnection
705-
* plan, in case there is no live nodes available according to LBP. Experimental.
704+
* Whether to append the original contact points held by MetadataManager to the control
705+
* connection's reconnection plan, after the live nodes reported by the load balancing policy.
706+
* Defaults to {@code true}.
707+
*
708+
* <p>This is the driver's DNS re-resolution path. A metadata node holds an address resolved once
709+
* and never re-resolved, the node the control connection reached included; a contact point given
710+
* as a hostname is kept unresolved (the default) and looked up again through Netty's resolver on
711+
* each connect, so once the live nodes are exhausted the contact points find a cluster that moved
712+
* to new addresses, as soon as the JVM's DNS cache ({@code networkaddress.cache.ttl}) has
713+
* expired. A resolved contact point ({@code advanced.resolve-contact-points = true}, or a
714+
* programmatic resolved address) is appended as it is and never re-resolved.
715+
*
716+
* <p>Skipped when the topology monitor re-resolves node addresses on its own ({@code
717+
* TopologyMonitor#reresolvesNodeAddresses()}: the Cloud SNI proxy, and client routes with full
718+
* route coverage), unless the live-node plan is empty.
706719
*
707720
* <p>Value-type: boolean
708721
*/

core/src/main/java/com/datastax/oss/driver/api/core/config/OptionsMap.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,7 @@ protected static void fillWithDriverDefaults(OptionsMap map) {
369369
map.put(TypedDriverOption.CONTROL_CONNECTION_AGREEMENT_INTERVAL, Duration.ofMillis(200));
370370
map.put(TypedDriverOption.CONTROL_CONNECTION_AGREEMENT_TIMEOUT, Duration.ofSeconds(10));
371371
map.put(TypedDriverOption.CONTROL_CONNECTION_AGREEMENT_WARN, true);
372-
map.put(TypedDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS, false);
372+
map.put(TypedDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS, true);
373373
map.put(TypedDriverOption.PREPARE_ON_ALL_NODES, true);
374374
map.put(TypedDriverOption.REPREPARE_ENABLED, true);
375375
map.put(TypedDriverOption.REPREPARE_CHECK_SYSTEM_TABLE, false);

core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,15 @@ public String toString() {
600600
public static final TypedDriverOption<Boolean> CONTROL_CONNECTION_AGREEMENT_WARN =
601601
new TypedDriverOption<>(
602602
DefaultDriverOption.CONTROL_CONNECTION_AGREEMENT_WARN, GenericType.BOOLEAN);
603-
/** Whether to forcibly try original contacts if no live nodes are available */
603+
/**
604+
* Whether to append the original contact points to the control connection's reconnection plan,
605+
* after the live nodes reported by the load balancing policy (defaults to {@code true}).
606+
*
607+
* <p>A contact point given as a hostname is kept unresolved and looked up again on each connect,
608+
* so this is how the driver picks up changed DNS records once the live nodes are exhausted. It is
609+
* skipped for a topology monitor that re-resolves node addresses on its own, unless the live-node
610+
* plan is empty.
611+
*/
604612
public static final TypedDriverOption<Boolean> CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS =
605613
new TypedDriverOption<>(
606614
DefaultDriverOption.CONTROL_CONNECTION_RECONNECT_CONTACT_POINTS, GenericType.BOOLEAN);

core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java

Lines changed: 96 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@
6161
import java.util.List;
6262
import java.util.Map;
6363
import java.util.Map.Entry;
64-
import java.util.Objects;
6564
import java.util.Queue;
6665
import java.util.WeakHashMap;
6766
import java.util.concurrent.CompletableFuture;
@@ -459,6 +458,19 @@ private void connect(
459458
connect(nodes, errors, onSuccess, onFailure);
460459
} else {
461460
LOG.debug("[{}] New channel opened {}", logPrefix, channel);
461+
// Identify the node before the channel becomes visible to anything else.
462+
// Every read of channel.getEndPoint() from here on -- the topology monitor's
463+
// three node-refresh methods included -- must see the same endpoint: if one
464+
// refresh identified the control node by the address it was dialled at and
465+
// the next by the address it answered on, the second would compare the two
466+
// (a blocking lookup, for a mixed resolved/unresolved pair) and then rewrite
467+
// the node's endpoint and clear its metrics.
468+
EndPoint identity =
469+
context.getTopologyMonitor().connectedNodeEndPoint(channel);
470+
if (identity != null && identity != channel.getEndPoint()) {
471+
channel.setEndPoint(identity);
472+
LOG.debug("[{}] Control channel identified as {}", logPrefix, identity);
473+
}
462474
DriverChannel previousChannel = ControlConnection.this.channel;
463475
ControlConnection.this.channel = channel;
464476
controlNodeState = new ControlNodeState(null, node);
@@ -499,6 +511,42 @@ private void connect(
499511
node,
500512
new Exception("Channel closed during endpoint resolve")));
501513
connect(nodes, newErrors, onSuccess, onFailure);
514+
} else if (isUnusableForControl(resolvedNode)) {
515+
// A contact point's identity is only known once the resolve
516+
// above completes, and events name the metadata node rather than
517+
// the placeholder we dialled, so isControlNode() cannot match the
518+
// two -- and comparing their endpoints there would mean the
519+
// blocking lookup this path exists to avoid. So re-run the
520+
// pre-connect check against the node we actually identified.
521+
// lastNodeDistance/lastNodeState are cumulative, so this also
522+
// catches an event that arrived before the channel opened, which
523+
// the check on the placeholder never saw.
524+
controlNodeState = ControlNodeState.NONE;
525+
LOG.debug(
526+
"[{}] New channel opened ({}) but {} is ignored or forced "
527+
+ "down, closing and trying next node",
528+
logPrefix,
529+
channel,
530+
resolvedNode);
531+
// Null out before forceClose() so that onChannelClosed() does not
532+
// start a redundant reconnection on top of the connect() retry
533+
// below.
534+
ControlConnection.this.channel = null;
535+
channel.forceClose();
536+
// Recorded like every other reason a candidate was dropped: a
537+
// round in which all of them are dropped here would otherwise
538+
// fail with a causeless NoNodeAvailableException, saying nothing
539+
// about the channels that were opened and deliberately closed.
540+
List<Entry<Node, Throwable>> newErrors =
541+
(errors == null) ? new ArrayList<>() : errors;
542+
newErrors.add(
543+
new SimpleEntry<>(
544+
node,
545+
new Exception(
546+
"Control node "
547+
+ resolvedNode
548+
+ " is ignored or forced down")));
549+
connect(nodes, newErrors, onSuccess, onFailure);
502550
} else {
503551
controlNodeState = new ControlNodeState(resolvedNode, null);
504552
context
@@ -545,13 +593,13 @@ private CompletionStage<Node> resolveChannelNodeIfNeeded(
545593
.thenComposeAsync(
546594
nodeInfo -> {
547595
EndPoint resolvedEp = nodeInfo.getEndPoint();
548-
// Compared by reference, not equals(): DefaultEndPoint.equals resolves the
549-
// unresolved side of a mixed comparison, a blocking lookup on this admin executor,
550-
// and it would answer "equal" for the very case this exists for (a hostname contact
551-
// point identified by the address it reached), skipping the adoption. Adopting the
596+
// A monitor that builds its endpoints from the identity row rather than from the
597+
// socket (the Cloud SNI proxy, client routes) only knows the node's endpoint now;
598+
// connectedNodeEndPoint() left the channel's alone for those. Compared by
599+
// reference, not equals(): DefaultEndPoint.equals resolves the unresolved side of
600+
// a mixed comparison, a blocking lookup on this admin executor. Adopting the
552601
// monitor's instance also makes the channel and the metadata node share it, so
553-
// every
554-
// later comparison of the two short-circuits on identity.
602+
// every later comparison of the two short-circuits on identity.
555603
if (resolvedEp != null && resolvedEp != channel.getEndPoint()) {
556604
channel.setEndPoint(resolvedEp);
557605
LOG.debug("[{}] Control channel endpoint upgraded to {}", logPrefix, resolvedEp);
@@ -694,12 +742,47 @@ private boolean isControlNode(Node eventNode) {
694742
&& eventNode.getHostId().equals(state.current.getHostId())) {
695743
return true;
696744
}
697-
if (state.current == null
698-
&& state.pending != null
699-
&& Objects.equals(eventNode.getEndPoint(), state.pending.getEndPoint())) {
700-
return true;
701-
}
702-
return false;
745+
// Reference identity, not endpoint equality: the reconnection plan now appends the
746+
// unresolved contact-point nodes (fallback-to-original-contact-points defaults to true), so a
747+
// mixed resolved/unresolved pair is routine here, and DefaultEndPoint.equals resolves the
748+
// unresolved side of one -- a blocking lookup on this admin executor, during the very DNS
749+
// outage the fallback exists for. Nodes carry identity semantics (no equals override, and
750+
// lastNodeState/lastNodeDistance already key on the instance) and connect() stores the plan's
751+
// own node as pending, so an event about that node carries the same object.
752+
//
753+
// An event naming the metadata node for the host we are still identifying cannot be matched
754+
// here at all -- pending is the placeholder, and the two are different instances by
755+
// construction (MetadataManager.registerNode never reuses a contact-point node). Endpoint
756+
// equality appeared to cover that case, but only by paying the blocking lookup above.
757+
// connect() instead re-checks the identified node with isUnusableForControl() once the
758+
// resolve completes, which closes the window without any DNS.
759+
return state.current == null && state.pending != null && eventNode == state.pending;
760+
}
761+
762+
/**
763+
* Whether an event has already marked this node unusable for the control connection: the load
764+
* balancing policy ignored it, or it was removed or forced down.
765+
*
766+
* <p>Mirrors the two checks {@link #connect} runs before it adopts a freshly opened channel.
767+
* Those stay separate rather than calling this: they answer from records read before the
768+
* channel was opened, this one from the records as they stand now, and each keeps its own
769+
* diagnostic message.
770+
*
771+
* <p>Answers only for a node some event has named. Both records are keyed by node instance (and
772+
* weakly), so this sees a node the driver already knows -- either a query-plan node, or a host
773+
* id {@code MetadataManager#registerNode} found in the metadata and handed back. It cannot see
774+
* a node registered fresh from a contact point: {@code registerNode} mints a new instance for
775+
* an unknown host id and re-inserts it into the metadata before this runs, so a host the driver
776+
* had removed is resurrected rather than rejected. Detecting that needs a removal record keyed
777+
* by host id, with a defined lifetime, which this check deliberately does not attempt:
778+
* rejecting every host id absent from the metadata would also reject the genuinely new nodes of
779+
* a cluster that moved, which is the case the fallback exists to recover.
780+
*/
781+
private boolean isUnusableForControl(Node node) {
782+
NodeState state = lastNodeState.get(node);
783+
return lastNodeDistance.get(node) == NodeDistance.IGNORED
784+
|| (lastNodeState.containsKey(node)
785+
&& (state == null /*(removed)*/ || state == NodeState.FORCED_DOWN));
703786
}
704787

705788
private void onDistanceEvent(DistanceEvent event) {

core/src/main/java/com/datastax/oss/driver/internal/core/metadata/ClientRoutesTopologyMonitor.java

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,14 @@
2121
import com.datastax.oss.driver.api.core.config.ClientRoutesConfig;
2222
import com.datastax.oss.driver.api.core.config.DefaultDriverOption;
2323
import com.datastax.oss.driver.api.core.metadata.EndPoint;
24+
import com.datastax.oss.driver.api.core.metadata.Node;
2425
import com.datastax.oss.driver.internal.core.adminrequest.AdminRequestHandler;
2526
import com.datastax.oss.driver.internal.core.adminrequest.AdminResult;
2627
import com.datastax.oss.driver.internal.core.adminrequest.AdminRow;
2728
import com.datastax.oss.driver.internal.core.channel.DriverChannel;
2829
import com.datastax.oss.driver.internal.core.clientroutes.ClientRouteRecord;
2930
import com.datastax.oss.driver.internal.core.context.InternalDriverContext;
31+
import com.datastax.oss.driver.internal.core.util.AddressUtils;
3032
import com.datastax.oss.driver.shaded.guava.common.annotations.VisibleForTesting;
3133
import edu.umd.cs.findbugs.annotations.NonNull;
3234
import edu.umd.cs.findbugs.annotations.Nullable;
@@ -480,6 +482,45 @@ protected EndPoint buildNodeEndPoint(
480482
return new ClientRoutesEndPoint(this, hostId, broadcastInetAddress, fallback);
481483
}
482484

485+
@Override
486+
public boolean reresolvesNodeAddresses() {
487+
// A ClientRoutesEndPoint looks its route hostname up on every connect, but only while a route
488+
// exists for that host id; without one it falls back to a static, already-resolved address. So
489+
// this answers true only while every known node has a live route, and "every" has to mean at
490+
// least one: an empty node set (before the first refresh, or after every node was removed) must
491+
// not suppress the contact-point fallback at the one moment it is the only way back. A closed
492+
// monitor re-resolves nothing at all, however complete its cache still looks.
493+
if (closed) {
494+
return false;
495+
}
496+
// Routes first, nodes second, and the order is the safety property rather than a style
497+
// choice: the two are published independently, by components that do not share this thread,
498+
// so a pass necessarily mixes one snapshot with a possibly newer other. Read this way, a node
499+
// added since the routes were published is missing from them and reports false; read the other
500+
// way it would be missing from the node set instead, and its absence of a route would go
501+
// unnoticed -- claiming a re-resolution that node does not have.
502+
Map<UUID, ClientRouteRecord> routes = resolvedRoutesCache.get();
503+
// getNodes() is already keyed by host id, so iterating its keys is exactly "every known node",
504+
// and the node objects themselves are never asked anything.
505+
Map<UUID, Node> nodes = context.getMetadataManager().getMetadata().getNodes();
506+
if (nodes.isEmpty()) {
507+
return false;
508+
}
509+
for (UUID hostId : nodes.keySet()) {
510+
ClientRouteRecord route = routes.get(hostId);
511+
// A route reaches DNS only if its address holds a name: resolve() calls
512+
// InetAddress.getByName(), which hands an IP literal straight back, so a literal-valued
513+
// route re-resolves nothing and must not suppress the contact-point fallback -- that
514+
// fallback is the only way such a node ever learns a new address. The address column takes
515+
// either form and nothing validates which (scylladb/java-driver#1064), so decide it here,
516+
// per record and lexically, without a lookup.
517+
if (route == null || AddressUtils.isIpLiteral(route.getHostname())) {
518+
return false;
519+
}
520+
}
521+
return true;
522+
}
523+
483524
/**
484525
* Builds the CQL query to fetch client routes.
485526
*

core/src/main/java/com/datastax/oss/driver/internal/core/metadata/CloudTopologyMonitor.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,4 +44,12 @@ protected EndPoint buildNodeEndPoint(
4444
UUID hostId = Objects.requireNonNull(row.getUuid("host_id"));
4545
return new SniEndPoint(cloudProxyAddress, hostId.toString());
4646
}
47+
48+
@Override
49+
public boolean reresolvesNodeAddresses() {
50+
// Every node is reached through the SNI proxy, whose hostname SniEndPoint#resolve() looks up on
51+
// every call, so addresses stay current on their own: appending the contact points as a DNS
52+
// fallback would add nothing, and could resurrect nodes this monitor has removed.
53+
return true;
54+
}
4755
}

core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.java

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ public CompletionStage<NodeInfo> getChannelNodeInfo(DriverChannel channel) {
351351
if (closeFuture.isDone()) {
352352
return CompletableFutures.failedFuture(new IllegalStateException("closed"));
353353
}
354-
EndPoint localEndPoint = connectedNodeEndPoint(channel);
354+
EndPoint localEndPoint = channel.getEndPoint();
355355
return query(channel, buildQuery(localColumns, "system.local", "key='local'"))
356356
.thenApply(
357357
result -> {
@@ -626,8 +626,8 @@ protected DefaultNodeInfo.Builder nodeInfoBuilder(
626626
}
627627

628628
/**
629-
* The endpoint to register the node at the other end of {@code channel} under, when the node is
630-
* identified for the first time.
629+
* The endpoint to identify the node at the other end of {@code channel} by, applied to the
630+
* channel by {@code ControlConnection} when it adopts it, before any query is sent.
631631
*
632632
* <p>A contact point's endpoint is an unresolved name by default, and that name is not the
633633
* node's: every node ever reached through the contact point would be registered under it, and
@@ -640,11 +640,14 @@ protected DefaultNodeInfo.Builder nodeInfoBuilder(
640640
*
641641
* <p>Anything else is returned as configured: a resolved endpoint ({@code resolve-contact-points
642642
* = true}, or a programmatic resolved address), a third-party {@link EndPoint}, or a channel
643-
* whose remote address is not an {@link InetSocketAddress}. For the default deployment this moves
644-
* the control node's metric name from the contact point's name to its own address, once, and pool
645-
* connections opened to it later verify TLS against that address, like every peer's.
643+
* whose remote address is not an {@link InetSocketAddress}. Those already name a node, so there
644+
* is nothing to derive -- a resolved contact point keeps identifying the control node by the name
645+
* it was configured with, deliberately. For the default deployment this moves the control node's
646+
* metric name from the contact point's name to its own address, once, and pool connections opened
647+
* to it later verify TLS against that address, like every peer's.
646648
*/
647-
private static EndPoint connectedNodeEndPoint(DriverChannel channel) {
649+
@Override
650+
public EndPoint connectedNodeEndPoint(DriverChannel channel) {
648651
EndPoint configured = channel.getEndPoint();
649652
if (!(configured instanceof DefaultEndPoint)
650653
|| !((DefaultEndPoint) configured).resolve().isUnresolved()) {

0 commit comments

Comments
 (0)