Copilot commented on code in PR #17348:
URL: https://github.com/apache/iotdb/pull/17348#discussion_r2980156831
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/common/MPPQueryContext.java:
##########
@@ -402,4 +402,8 @@ public boolean isUserQuery() {
public void setUserQuery(boolean userQuery) {
this.userQuery = userQuery;
}
+
+ public String getClientHostName() {
Review Comment:
`getClientHostName()` dereferences `session` without a null check, but this
class already documents that many callers pass a null SessionInfo (see the TODO
above). That means SHOW QUERIES (via QueryExecution.getClientHostname()) can
NPE for queries created with a null session. Consider returning a safe default
when `session` is null (e.g., empty string/"UNKNOWN") or enforcing non-null
SessionInfo at construction time before exposing this method.
```suggestion
public String getClientHostName() {
if (session == null || session.getCliHostname() == null) {
return "UNKNOWN";
}
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/common/SessionInfo.java:
##########
@@ -65,16 +69,22 @@ public ClientVersion getVersion() {
return version;
}
+ public String getCliHostname() {
+ return cliHostname;
+ }
Review Comment:
The field/method naming is inconsistent with what is actually stored:
`SessionManager` passes `IClientSession.getClientAddress()` (an IP/client id),
but the SessionInfo field is called `cliHostname` and exposed as
`getCliHostname()`. This is easy to misinterpret (hostname vs address/ip) and
leaks into APIs like `getClientHostName()`/`getClientHostname()`. Please rename
to something precise like `clientAddress`/`clientIp` (and adjust related
getters) to match the SHOW QUERIES `ClientIp` column.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/plan/node/source/ShowQueriesNode.java:
##########
@@ -32,15 +32,16 @@
import java.nio.ByteBuffer;
import java.util.List;
import java.util.Objects;
+import java.util.stream.Collectors;
+
+import static
org.apache.iotdb.db.queryengine.common.header.ColumnHeaderConstant.showQueriesColumnHeaders;
public class ShowQueriesNode extends VirtualSourceNode {
public static final List<String> SHOW_QUERIES_HEADER_COLUMNS =
- ImmutableList.of(
- ColumnHeaderConstant.QUERY_ID,
- ColumnHeaderConstant.DATA_NODE_ID,
- ColumnHeaderConstant.ELAPSED_TIME,
- ColumnHeaderConstant.STATEMENT);
+ showQueriesColumnHeaders.stream()
+ .map(ColumnHeader::getColumnName)
+ .collect(Collectors.toList());
Review Comment:
SHOW_QUERIES_HEADER_COLUMNS used to be an ImmutableList; it is now a mutable
`ArrayList` from `Collectors.toList()`. Since this is a public static constant,
keeping it immutable helps prevent accidental modification and potential
concurrency issues. Consider collecting into an unmodifiable list (e.g.,
`Collectors.toUnmodifiableList()` or `ImmutableList.copyOf(...)`).
```suggestion
.collect(ImmutableList.toImmutableList());
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/common/SessionInfo.java:
##########
@@ -65,16 +69,22 @@ public ClientVersion getVersion() {
return version;
}
+ public String getCliHostname() {
+ return cliHostname;
+ }
+
public static SessionInfo deserializeFrom(ByteBuffer buffer) {
long sessionId = ReadWriteIOUtils.readLong(buffer);
String userName = ReadWriteIOUtils.readString(buffer);
ZoneId zoneId =
ZoneId.of(Objects.requireNonNull(ReadWriteIOUtils.readString(buffer)));
- return new SessionInfo(sessionId, userName, zoneId);
+ String cliHostname = ReadWriteIOUtils.readString(buffer);
+ return new SessionInfo(sessionId, userName, zoneId, cliHostname);
}
public void serialize(DataOutputStream stream) throws IOException {
ReadWriteIOUtils.write(sessionId, stream);
ReadWriteIOUtils.write(userName, stream);
ReadWriteIOUtils.write(zoneId.getId(), stream);
+ ReadWriteIOUtils.write(cliHostname, stream);
Review Comment:
SessionInfo serialization format was changed by appending `cliHostname`
without any version/tagging. This will break deserialization of
FragmentInstance/SessionInfo data produced by older nodes (BufferUnderflow /
corrupted reads) during rolling upgrades or mixed-version clusters. Consider
adding backward-compatible decoding (e.g., conditional read based on remaining
bytes / protocol version) or introducing an explicit serialization version/flag
before adding new fields.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/IQueryExecution.java:
##########
@@ -66,6 +66,10 @@ public interface IQueryExecution {
*/
long getTotalExecutionTime();
+ String getClientHostname();
+
Review Comment:
The new API name `getClientHostname()` doesn’t match the SHOW QUERIES column
name `ClientIp`/the underlying value (`IClientSession.getClientAddress()`).
This mismatch makes it unclear whether the value is an IP, hostname, or client
id (MQTT/REST). Consider renaming this to something unambiguous like
`getClientAddress()`/`getClientIp()` (or update the column header to match the
intended semantics).
```suggestion
/**
* Returns the client network address (for example, IP address or
hostname).
*/
String getClientAddress();
/**
* @deprecated Use {@link #getClientAddress()} instead. This method name
is misleading
* because it may return an IP address or other address, not strictly
a hostname.
*/
@Deprecated
default String getClientHostname() {
return getClientAddress();
}
```
--
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]
