Repository navigation
Filter offline DataNode URLs and add Java client seed fallback - #18921
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Session reopening can strand discovery on offline seeds, and refresh synchronization can delay shutdown through connection attempts.
2 open issues
What changed in this PR
Addresses offline-node discovery delays while preserving Java client recovery through configured seeds during rolling restarts.
Changes:
- Filters offline DataNodes from
SHOW AVAILABLE URLS. - Adds discovered-first, seed-second recovery and coalesced discovery refreshes.
- Adds unit and cluster tests for filtering, recovery, and concurrent endpoint selection.
| File | Description |
|---|---|
| iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/plan/execution/config/metadata/ShowAvailableUrlsTaskTest.java | Tests status filtering and empty results. |
| iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/metadata/ShowAvailableUrlsTask.java | Excludes offline DataNodes. |
| iotdb-client/session/src/test/java/org/apache/iotdb/session/SessionSeedNodeFallbackTest.java | Tests seed recovery and discovery snapshots. |
| iotdb-client/session/src/main/java/org/apache/iotdb/session/SessionConnection.java | Adds bounded, deduplicated seed fallback. |
| iotdb-client/session/src/main/java/org/apache/iotdb/session/Session.java | Preserves initial recovery addresses. |
| iotdb-client/session/src/main/java/org/apache/iotdb/session/pool/SessionPool.java | Copies configured seed URLs. |
| iotdb-client/session/src/main/java/org/apache/iotdb/session/NodesSupplier.java | Adds background fallback and snapshot-safe selection. |
| iotdb-client/isession/src/main/java/org/apache/iotdb/isession/INodeSupplier.java | Adds seed and refresh hooks. |
| integration-test/src/test/java/org/apache/iotdb/session/it/SessionSeedNodeFallbackIT.java | Tests rolling restarts and omitted-seed recovery. |
Review effort: Balanced
| } | ||
|
|
||
| @Override | ||
| public synchronized void run() { |
There was a problem hiding this comment.
Confirmed, with one nuance about how much of this is new. Measured with close() called 300 ms into a refresh (A/B, same harness as above):
| candidates | base close() |
head close() |
|---|---|---|
3 hosts that accept TCP but never answer, connectionTimeoutInMs = 1500 |
1251 ms | 4274 ms |
3 black-holed hosts (SYN dropped), connectionTimeoutInMs = 2000 |
0 ms | 5726 ms |
- Base already waits for an in-flight attempt whose TCP connection has been accepted.
ThriftConnection.close()sendscloseSessionthrough the synchronized client, so it waits for the pendingopenSession. While the TCP connect itself is still pending,TSocket.isOpen()is false,close()is a no-op, and base returns immediately. The refresh then carries on in the background: about 6 s in both versions. - Head waits for every remaining candidate (discovered + seeds) plus the final
SHOW AVAILABLE URLS.run()holds the monitor for the whole loop, and the loop never rechecksclosed.
With the default connectionTimeoutInMs = 0, a peer that accepts but never answers blocks close() forever on both base and head; that part is pre-existing. Head extends the wait to every candidate. With timeout 0, each black-holed host costs the OS SYN timeout: about 21 s on Windows, about 127 s on Linux. Session.close(), re-open() and SessionPool.close() all go through supplier.close(). Session.close() and SessionPool.close() are synchronized, so their callers are blocked as well.
Suggestion:
- Don't hold the monitor that
close()needs across network I/O. Serialize refreshes with a separate lock ortryLock. - Check
closedbefore each candidate and again after a successful connect. - Let
close()close the in-flight transport directly so a blocked attempt is aborted.
Please keep the race this synchronized fixes as a side effect: on base, run() can still create and leak a connection after close().
| } else { | ||
| return SessionUtils.parseSeedNodeUrls(nodeUrls); | ||
| } | ||
| return SessionUtils.parseSeedNodeUrls(initialNodeUrls); |
There was a problem hiding this comment.
Confirmed with an A/B run: the same test class against base bacbd5a6dc and head c600593, reusing the real-Thrift TestNode harness from SessionSeedNodeFallbackTest.
Scenario: nodeUrls = [A]; discovery returns [A, B]; A stops and the server now advertises only B; the default connection's reconnect() moves to B; then close(), open() and 10 executeQueryStatement calls.
| base | head | |
|---|---|---|
| new supplier's list after reopen | [B] |
[A] (A is unreachable, so it never refreshes) |
| connection attempts to A during the 10 queries | 0 | 10 |
On head, getQueryEndPoint() returns A for every query, so each query first tries to open a connection to A before falling back to the default connection. Against a host that is really down, each of those attempts is a full connect timeout. That is the #18633 symptom, now for every query instead of 1/N. Host/port sessions that failed over or were redirected behave the same way. SessionPool is not affected because pooled sessions share the pool's supplier.
Suggested fix: bootstrap discovery from defaultEndPoint (when set) followed by the initial URLs, and keep only the initial URLs as permanent seeds. testSessionRetainsInitialUrlsAfterDefaultEndpointChanges currently asserts the regressing behaviour, so it needs to change. A close/reopen-after-failover test would lock the fix in.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Review summary
The direction looks right. Filtering offline DataNodes on the server, plus a seed fallback on the client, fixes the #18633 scenario. SessionSeedNodeFallbackTest passes locally (JDK 21) and in CI (10/10).
I reran the suspicious paths with one test class against base bacbd5a6dc and head c600593, reusing the real-Thrift TestNode harness from SessionSeedNodeFallbackTest:
| Scenario | base | head |
|---|---|---|
close() during a refresh; 3 hosts that accept TCP but never answer; 1.5 s timeout |
blocked 1251 ms | blocked 4274 ms |
close() during a refresh; 3 black-holed hosts; 2 s timeout |
blocked 0 ms | blocked 5726 ms |
nodeUrls = [A]; A stops → fail over to B → close()/open() → 10 queries |
list [B], 0 attempts to A |
list stuck at [A], 10 of 10 queries try A first |
every SHOW AVAILABLE URLS row is 0.0.0.0 |
list = seeds; queries round-robin | list []; no query endpoint |
SHOW AVAILABLE URLS fails on every node; one refresh |
1 failed query, 0 new sessions, ~1.0 s | 3 failed queries + 2 new sessions, ~3.0 s |
seeds as localhost, discovery returns 127.0.0.1; all nodes down; one reconnect() |
6 attempts | 12 attempts |
Should fix before merge. Both Copilot threads hold up; my replies there have the details and suggested fixes.
- After a failover, reopening a
Sessionseeds discovery with the offline initial node (Session.javaL608). synchronized run()makesclose()wait for every candidate (NodesSupplier.javaL213).
Worth changing or documenting. Details are inline.
- A query failure on a reachable node is treated like an unreachable node.
- Publishing an empty discovery result changes behaviour in two cases: a wildcard
dn_rpc_address, and the window right after a ConfigNode leader change. - A node in graceful shutdown (
ReadOnly+Stopping) is still advertised after its client RPC port has closed. - Seeds and discovered endpoints are deduplicated by literal
TEndPointequality.
Compatibility. The server-side filter applies to every client, but only the updated Java SDK gets the seed fallback. Older Java SDKs and the C++ client have no fallback: C++ NodesSupplier::refreshEndpointList() replaces its list, and SessionConnection::reconnect() only uses the discovered list. Until now these clients kept Stopped nodes as recovery candidates. During a rolling restart they now only have the nodes that were online at the last refresh. The #18633 thread already noted that this server-side change "involves server-side API semantics and compatibility with other clients". Please state the upgrade order (clients first) in the release note, or port the seed fallback to the C++ client.
Missing tests:
close()during a blocked refreshclose()/open()after a failover- all-wildcard rows
ReadOnly+StoppinginShowAvailableUrlsTaskTest
CI:
unit-test (17, ubuntu/windows, others)fails in the ConfigNode testsRestartDuringRemovePeerTestandRemovePeerRestartConsensusTest. These fail on master too (run 39260519861), so they are unrelated.dual-table-manual-basicfails inIoTDBPipePermissionIT.testSourcePermission. That test runs over JDBC + pipe and doesn't use Session discovery, so it looks unrelated; worth a re-run.
FYI, unrelated and pre-existing: in the C++ client, SessionConnection.h calls std::find(availableNodes->getEndPointList().begin(), availableNodes->getEndPointList().end(), ...). getEndPointList() returns by value, so begin() and end() come from two different temporaries. That is undefined behaviour on the retry path.
| candidates.addAll(seedNodes); | ||
| for (TEndPoint endPoint : candidates) { | ||
| if (attempted.add(endPoint)) { | ||
| if (createConnection(endPoint) && updateDataNodeList()) { |
There was a problem hiding this comment.
A query failure is treated like an unreachable node. createConnection(endPoint) && updateDataNodeList() moves on to the next candidate in both cases: when the node can't be reached, and when it was reached but SHOW AVAILABLE URLS failed.
So a cluster-wide query failure opens a new session on every discovered node and every seed. One example is a cluster with no ConfigNode leader, where showDataNodes fails on every DataNode. This happens on every refresh of every Session, while the monitor from the L213 thread is held.
Measured for one refresh with 3 nodes whose query fails after 1 s:
- base: 1 failed query, 0 new sessions, ~1.0 s
- head: 3 failed queries + 2 new sessions, ~3.0 s
Suggestion: move to the next candidate only when the connection fails. If a reachable node fails the query, end this round and keep the current snapshot.
| availableNodes = res; | ||
| } | ||
| // An empty successful result is authoritative too; seeds remain available for recovery. | ||
| availableNodes = Collections.unmodifiableList(res); |
There was a problem hiding this comment.
An empty result used to mean "no information"; now it means "no nodes". In practice an empty filtered result appears while the responding node itself is alive:
- With
dn_rpc_address=0.0.0.0, the wildcard filter above drops every row. The officialdocker/src/main/Dockerfile-1c1dsets exactly this. - Right after a ConfigNode leader change, node statistics default to
Unknown(NodeStatistics.generateDefaultNodeStatistics()). Until the first heartbeats arrive, the server filters out every node.
Measured with all rows 0.0.0.0: base keeps the seeds, so getQueryEndPoint() keeps round-robining. Head publishes [], and getQueryEndPoint() returns empty.
Consequences:
- Queries are no longer spread across nodes.
- Some RPCs go through
callWithRetryAndReconnect: createTimeseries, templates,setTimeZone, raw/last/aggregation queries and others. For these, the!availableNodes.get().contains(this.endPoint)check (SessionConnection.javaL1034) is always true, so every retryable status forces a reconnect plus a refresh request. - The empty snapshot stays until the next periodic refresh, up to 60 s.
Suggestion: if every row was a wildcard, treat the result as no information and keep the previous snapshot. If the server genuinely returned no rows, keep the previous snapshot, or at least publish the responder (client.endPoint). testEmptyDiscoveryFallsBackWithoutLosingSeeds and testQuerySelectionDuringEmptyDiscoveryRefresh would need to follow.
| if (NodeStatus.Removing.getStatus().equals(status) | ||
| || NodeStatus.Unknown.getStatus().equals(status) | ||
| || NodeStatus.Stopped.getStatus().equals(status)) { |
There was a problem hiding this comment.
A node in graceful shutdown is still advertised. DataNodeShutdownHook.run() does three things in this order:
- It stops
ExternalRPCService(L117). - It sets
ReadOnlywith reasonNodeStatus.STOPPING(L120-122). - It reports
Stoppedto the ConfigNode leader at the very end, afterDataNode.stop()(L203).
Between steps 2 and 3 the node runs the WAL flush, syncCloseAllProcessor() and Ratis snapshots, and waits for pipes. Heartbeats keep reporting ReadOnly/Stopping, so this filter still returns the node although its client port is already closed. SessionSeedNodeFallbackIT waits for Stopped before calling run(), so it does not cover this window.
TDataNodeInfo already carries statusReason; NodeManager.getRegisteredDataNodeInfoList() fills it from the heartbeat sample. This filter could also skip ReadOnly when NodeStatus.STOPPING.equals(dataNodeInfo.getStatusReason()). An older ConfigNode that doesn't send the reason keeps today's behaviour. A ShowAvailableUrlsTaskTest case for ReadOnly + Stopping would cover it.
| endPointList.addAll(((INodeSupplier) availableNodes).getSeedNodes()); | ||
| } | ||
| // Try every discovered endpoint before falling back to seeds, once per endpoint per round. | ||
| endPointList = new ArrayList<>(new LinkedHashSet<>(endPointList)); |
There was a problem hiding this comment.
Deduplication is by literal TEndPoint. If nodeUrls use host names and dn_rpc_address uses IPs (or the reverse), every node appears twice: here, and in the candidates of NodesSupplier.run().
Measured with seeds localhost:p1/p2 and discovery returning 127.0.0.1:p1/p2, all nodes down: one reconnect() makes 12 attempts on head versus 6 on base. Each attempt can be a full connect timeout.
There are two more side effects:
- Connecting through the host-name form of an already discovered node also calls
requestRefresh(). - After that,
this.endPointis never in the discovered list, socallWithRetryAndReconnectreconnects on every retryable status.
Suggestion: document that seeds should use the same address form as dn_rpc_address, or deduplicate on resolved addresses.
| Collections.rotate(endPointList, -random.nextInt(endPointList.size())); | ||
| } | ||
| if (availableNodes instanceof INodeSupplier) { | ||
| endPointList.addAll(((INodeSupplier) availableNodes).getSeedNodes()); |
There was a problem hiding this comment.
nit: seeds are appended in a fixed order, and for SessionPool the shared supplier's seeds are never shuffled. When all discovered endpoints fail, every pooled session falls back to the same first live seed, and nothing rebalances them afterwards. Rotating the seed part randomly in each round, as already done for the discovered part, would spread that load.
| throw new IllegalArgumentException(SessionMessages.NODE_URLS_EMPTY); | ||
| } | ||
| this.nodeUrls = builder.nodeUrls; | ||
| this.nodeUrls = Collections.unmodifiableList(new ArrayList<>(builder.nodeUrls)); |
There was a problem hiding this comment.
nit: only the builder constructor copies nodeUrls. The List<String> constructors (L506) still keep the caller's list, and later constructNewSession() calls read it. Copying there too would match "independent copies" in the description.
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
Round 2 on c240197
I reran the round-1 A/B harness on this commit. Every round-1 item is fixed:
close()during a refresh returns in 0 ms in all three cases. That includes the timeout-0 peer that accepts but never answers, which hangs on base.- Reopen after a failover makes 0 attempts to the stopped node.
- A cluster-wide query failure costs 1 query and no new sessions per refresh.
- Wildcard and empty results keep the previous snapshot.
ReadOnly+Stoppingis filtered.- Seeds are rotated, and the pool copies the URL list.
Locally, SessionSeedNodeFallbackTest passes 18/18 and ThriftConnectionLifecycleTest passes 3/3. Dropping the explicit closeSession is fine, because the server's deleteContext() → handleClientExit() closes the session.
One new issue and three nits, inline:
- A node that keeps failing the discovery query now pins discovery (
NodesSupplier.javaL420-426). run()became best-effort (tryLock), butSessionSeedNodeFallbackITstill calls it as a synchronous refresh.- nit, pre-existing: with
enableAutoFetch(false), a reopen shrinks the static list to one endpoint. - nit: placement of the
RELEASE_NOTES.mdsection.
| if (!(e.getCause() instanceof TApplicationException)) { | ||
| throw e; | ||
| } | ||
| // A Thrift application error also means the peer responded; it is not a transport failure. | ||
| } catch (Exception e) { | ||
| // A query error (for example no ConfigNode leader) is not evidence of a dead endpoint. | ||
| // Keep this connection and snapshot, and let the next scheduled refresh retry the query. |
There was a problem hiding this comment.
A node that keeps failing the discovery query now pins discovery. Both branches here keep the connection and end the round. The next scheduled refresh starts from the same client, so discovery never moves away from this node while it stays reachable.
A typical case is a DataNode that is cut off from the ConfigNode while clients can still reach it. The ConfigNode marks it Unknown, which is exactly the node the new server-side filter is meant to drop.
A/B with the same harness:
- Discovery first connects to A while A is healthy; snapshot
[B, C, A]. - Then only A's query keeps failing, while B and C answer
[B, C]. - Three refreshes follow.
| snapshot after 3 refreshes | queries A / B / C | |
|---|---|---|
base bacbd5a6dc |
[B, C] |
1 / 2 / 0 |
c600593 |
[B, C] |
1 / 3 / 0 |
c240197 |
[B, C, A] (frozen) |
3 / 0 / 0 |
While discovery is pinned, the snapshot is frozen: A stays in the query rotation, and restarts of other nodes are not picked up. testQueryStatusFailureDoesNotRotateConnections and testQueryApplicationFailureDoesNotRotateConnections assert this behaviour.
Ending the round on a query error is right; it removed the sweep. The problem is keeping the connection for the next round. Suggestion: after a query error, close the connection and start the next round from a different candidate, for example by putting the failed endpoint last. A cluster-wide failure then costs at most one new connection and one query per refresh, which is still bounded.
|
|
||
| @Override | ||
| public void run() { | ||
| if (closed || !refreshLock.tryLock()) { |
There was a problem hiding this comment.
run() is now best-effort. With tryLock(), a direct call returns immediately, without refreshing, while another refresh holds the lock. Scheduled and requested refreshes share the session/pool executor, so they are already serialized and unaffected. Callers outside that executor are affected.
SessionSeedNodeFallbackIT calls sessionSupplier.run() / poolSupplier.run() directly and asserts right afterwards (L100-101 and L108-109). With synchronized in c600593, such a call waited for the in-flight refresh and then refreshed again. Now, if a scheduled refresh happens to be running, it can return before the snapshot changes. The unit tests already poll (awaitSnapshot); polling in the IT as well would keep it deterministic.
| return defaultEndPoint == null | ||
| ? SessionUtils.parseSeedNodeUrls(initialNodeUrls) | ||
| : Collections.singletonList(defaultEndPoint.deepCopy()); |
There was a problem hiding this comment.
nit, pre-existing: with enableAutoFetch(false), open() builds new DummyNodesSupplier(getNodeUrls()), and DummyNodesSupplier has no seeds. After a failover and close()/open(), this method returns only [defaultEndPoint], so the static list shrinks to a single endpoint.
Checked with nodeUrls = [A, B]: after a failover to B and a reopen, the static list is [B]. When B then stops, reconnect() fails even though A is back. Base behaves the same way. c600593 happened to keep the full list, because getNodeUrls() always returned initialNodeUrls there.
Passing [defaultEndPoint] + initialNodeUrls (deduplicated) to DummyNodesSupplier would keep failover working after a reopen.
|
|
||
| --> | ||
|
|
||
| # Unreleased |
There was a problem hiding this comment.
nit: on master, RELEASE_NOTES.md only changes in the release-info PRs (#18164, #17468, #17275, ...), so an # Unreleased section added by a feature PR may be moved or rewritten by the release manager. The upgrade-order note (clients before servers) might fit better in the PR description or the user docs, where it gets picked up at release time.
Close query-failed connections and defer their endpoints across refresh rounds while preserving the last usable snapshot and bounded query retries. Retain all initial addresses when reopening a Session without discovery. Poll for integration-test snapshots and keep upgrade guidance in the PR.
Description
Fixes #18633.
Supersedes #18920.
Avoid repeatedly routing Java SDK queries to DataNodes that the server already considers offline, while retaining recovery through the client's initial addresses when a rolling restart makes the latest discovery snapshot stale.
Rolling restarts and recovery
A client can discover only nodes 2 and 3 while node 1 is stopped. Node 1 may recover before the next successful refresh, while later attempts to the cached endpoints fail. The Java client first tries the discovered endpoints, then falls back to its initial
nodeUrls, so the omitted node remains a recovery candidate if it was configured as a seed.Business reconnection and background discovery prefer discovered endpoints before fallback seeds. Background discovery defers candidates that returned query errors, as described below. Business reconnection rotates both tiers independently, and background discovery rotates its fallback seed tier. Literal duplicate endpoints are tried once per business reconnection round. A successful business reconnection through a seed requests a coalesced background refresh.
Discovery and lifecycle behavior
SHOW AVAILABLE URLSexcludesUnknown,Stopped,Removing, andReadOnlynodes with reasonStopping. OtherReadOnlynodes, including responses from older ConfigNodes without a reason field, remain eligible.Upgrade and address compatibility
Upgrade Java clients to a version with seed fallback before upgrading servers. Configure several stable seeds and keep at least one reachable during a rolling upgrade. The server filter affects all clients; older Java SDKs and the C++ client do not acquire fallback through a server upgrade. Their maintenance plan must retain a reachable discovered endpoint, or use stable explicit endpoints with automatic discovery disabled until the client supports fallback.
Deduplication remains based on literal host/IP and port. The builder Javadocs recommend a consistent address representation between seeds and advertised endpoints; mixing aliases such as
localhostand127.0.0.1can still cause duplicate attempts and extra refreshes/reconnections. DNS resolution is not used to infer permanent node identity. Wildcard bind addresses should be replaced with reachable addresses innodeUrls.Recovery requires at least one reachable configured seed when every discovered endpoint fails. Endpoint backoff for nodes reported as
Runningbut unreachable from a particular client is outside this change.Validation
Validated on JDK 17 in an isolated worktree:
ShowAvailableUrlsTaskTesttests.openSessionor discovery query with timeout zero; aborting during or before TCP connect; disposing a transport created after closure; close/reopen after failover for bothnodeUrlsand single-host Sessions; recovery from persistent node-local status/application errors; bounded cluster-wide query failures and subsequent recovery; discovery through an omitted seed after query failures; static endpoint retention and a second failover after reopen; empty/all-wildcard snapshots; legacy SessionPool constructor mutation; bounded retries; andReadOnlyshutdown reasons.SessionSeedNodeFallbackITtests passed on a real 1 ConfigNode / 3 DataNode cluster: Session and SessionPool data queries during successive node restarts, and business/independent background recovery through a seed omitted from a deliberately stale discovery snapshot. The latter uses a metadata query with two DataNodes offline.mvn test-compile -DskipTestsandmvn test-compile -DskipTests -P with-zh-localepassed.The integration tests poll for the expected snapshot because a direct
NodesSupplier.run()call may return while another refresh holds the lock. The immediate business-triggered refresh is still checked independently without polling it manually.The rolling-restart integration test tolerates
IoTDBConnectionExceptiononly when closing stale business sessions during cleanup. Query and discovery failures are not ignored.This PR has:
Key changed/added classes
INodeSupplier,NodesSupplier,ThriftConnection,SessionConnection,Session,SessionPool,ShowAvailableUrlsTask, their builder documentation,SessionSeedNodeFallbackTest,ThriftConnectionLifecycleTest,ShowAvailableUrlsTaskTest, andSessionSeedNodeFallbackIT.