JackieTien97 commented on code in PR #17631:
URL: https://github.com/apache/iotdb/pull/17631#discussion_r3214496091
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/client/ConfigNodeClient.java:
##########
@@ -265,6 +265,13 @@ public ConfigNodeClient(
}
public void connect(TEndPoint endpoint, int timeoutMs) throws TException {
+ // Close old transport to avoid leaking TCP connections on the server side.
+ // Without this, redirect scenarios (Follower -> Leader) overwrite the
transport
+ // field and leave the old connection open, causing server-side RPC
threads to
+ // block indefinitely on the abandoned socket.
Review Comment:
Nit: This 4-line comment is somewhat verbose for what is essentially a
defensive `close()` before reassignment. The full context (redirect scenario,
server-side thread leak) is already well-documented in the commit message and
PR description. Consider trimming to a single line, e.g.:
```java
// Close existing transport before reassigning to prevent connection leaks.
```
The code itself (`if (transport != null) { transport.close(); }` before a
field reassignment) is a well-known resource-management pattern that most Java
developers will recognize on sight.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/client/ConfigNodeClient.java:
##########
@@ -265,6 +265,13 @@ public ConfigNodeClient(
}
public void connect(TEndPoint endpoint, int timeoutMs) throws TException {
+ // Close old transport to avoid leaking TCP connections on the server side.
+ // Without this, redirect scenarios (Follower -> Leader) overwrite the
transport
+ // field and leave the old connection open, causing server-side RPC
threads to
+ // block indefinitely on the abandoned socket.
+ if (transport != null) {
Review Comment:
Now that `connect()` always closes the old transport before creating a new
one, the existing `transport.close()` in `tryToConnect()` (lines 337-339 on
master, lines 344-346 in this PR) becomes redundant — when the round-robin loop
calls `connect(tryEndpoint, timeoutMs)`, the old transport will be closed at
entry.
The duplicate close is *harmless* (`TTransport.close()` is idempotent), but
having two call sites that both claim responsibility for transport cleanup is a
maintenance hazard. It's ambiguous whether `tryToConnect()` or `connect()` owns
the lifecycle.
Suggestion: remove the `if (transport != null) { transport.close(); }` block
in `tryToConnect()` so that `connect()` is the single owner of transport
teardown. This makes the contract clearer: **every** `connect()` call manages
its own cleanup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
