Copilot commented on code in PR #17358:
URL: https://github.com/apache/iotdb/pull/17358#discussion_r2992285295
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/thrift/impl/ClientRPCServiceImpl.java:
##########
@@ -241,6 +242,9 @@ public class ClientRPCServiceImpl implements
IClientRPCServiceWithHandler {
private static final int DEFAULT_MAX_TSBLOCK_SIZE_IN_BYTES =
TSFileDescriptor.getInstance().getConfig().getMaxTsBlockSizeInBytes();
+ private static final String NO_QUERY_EXECUTION_ERR_MSG =
+ "Query is not found, it may be killed by others, timeout or some other
runtime errors, you can see more details in server log.";
Review Comment:
This string literal appears to exceed the project's 100-character
line-length limit, which will likely fail Checkstyle. Please wrap/split the
message across multiple concatenated strings (or otherwise format it) to keep
each line within the limit.
```suggestion
"Query is not found, it may be killed by others, timeout or some other
runtime errors, you "
+ "can see more details in server log.";
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/thrift/impl/ClientRPCServiceImpl.java:
##########
@@ -1686,16 +1693,16 @@ public TSFetchResultsResp
fetchResults(TSFetchResultsReq req) {
return RpcUtils.getTSFetchResultsResp(getNotLoggedInStatus());
}
- TSFetchResultsResp resp =
RpcUtils.getTSFetchResultsResp(TSStatusCode.SUCCESS_STATUS);
-
queryExecution = COORDINATOR.getQueryExecution(req.queryId);
if (queryExecution == null) {
- resp.setHasResultSet(false);
- resp.setMoreData(true);
- return resp;
+ TSStatus noQueryExecutionStatus = new
TSStatus(QUERY_WAS_KILLED.getStatusCode());
+ noQueryExecutionStatus.setMessage(NO_QUERY_EXECUTION_ERR_MSG);
+ return RpcUtils.getTSFetchResultsResp(noQueryExecutionStatus);
}
+ queryExecution.updateCurrentRpcStartTime(startTime);
statementType = queryExecution.getStatementType();
+ TSFetchResultsResp resp =
RpcUtils.getTSFetchResultsResp(TSStatusCode.SUCCESS_STATUS);
try (SetThreadName queryName = new
SetThreadName(queryExecution.getQueryId())) {
Pair<TSQueryDataSet, Boolean> pair =
convertTsBlockByFetchSize(queryExecution, req.fetchSize);
Review Comment:
In this `fetchResults` path, the response's `moreData` flag is set using
`finished` (later in the method), which appears inverted compared to other
fetch-result paths in this class (they use `!finished`). This can cause clients
to think more data exists when the query is actually finished (or stop early if
they interpret it differently). Please double-check and align the `moreData`
semantics here.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/thrift/impl/ClientRPCServiceImpl.java:
##########
@@ -1147,15 +1151,18 @@ public TSFetchResultsResp
fetchResultsV2(TSFetchResultsReq req) {
finished = true;
return RpcUtils.getTSFetchResultsResp(getNotLoggedInStatus());
}
- TSFetchResultsResp resp =
RpcUtils.getTSFetchResultsResp(TSStatusCode.SUCCESS_STATUS);
queryExecution = COORDINATOR.getQueryExecution(req.queryId);
if (queryExecution == null) {
- resp.setHasResultSet(false);
- resp.setMoreData(false);
- return resp;
+ TSStatus noQueryExecutionStatus = new
TSStatus(QUERY_WAS_KILLED.getStatusCode());
Review Comment:
When a query execution cannot be found, returning
TSStatusCode.QUERY_WAS_KILLED is misleading because the query may be gone for
other reasons (e.g., finished/cleaned up or timed out). Consider using
TSStatusCode.NO_SUCH_QUERY (and/or TSStatusCode.QUERY_TIMEOUT when applicable)
so clients can distinguish a missing handle from an explicit kill.
```suggestion
TSStatus noQueryExecutionStatus =
new TSStatus(TSStatusCode.NO_SUCH_QUERY.getStatusCode());
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/ConfigExecution.java:
##########
@@ -72,6 +72,12 @@ public class ConfigExecution implements IQueryExecution {
private final StatementType statementType;
private long totalExecutionTime;
+ // -1 if previous rpc is finished and next client req hasn't come yet, unit
is ns
+ // it will be updated in fetchResult rpc
+ // currently, ConfigExecution will return result is just one call, so this
field is not used. But
+ // we will keep it for future use when ConfigExecution may return result in
multiple calls
+ private long startTimeOfCurrentRpc = System.nanoTime();
Review Comment:
`startTimeOfCurrentRpc` is mutated by `recordExecutionTime()` /
`updateCurrentRpcStartTime()` and read by `getTotalExecutionTime()`, but unlike
`QueryExecution` these methods are not synchronized and the field isn't
`volatile`/atomic. This can lead to racy/inaccurate total-time calculations
(and potentially incorrect cleanup decisions if future logic relies on `-1`).
Consider mirroring `QueryExecution` (synchronized methods or an `AtomicLong`),
and also update the comment that says the field is "not used" since
`getTotalExecutionTime()` already uses it.
```suggestion
// it will be updated in fetchResult rpc and used by
getTotalExecutionTime() to calculate
// the total execution time across RPC calls
private volatile long startTimeOfCurrentRpc = System.nanoTime();
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/Coordinator.java:
##########
@@ -300,6 +298,31 @@ public void cleanupQueryExecution(
}
}
+ /**
+ * cWe need to reclaim resources from queries that have exceeded their
timeout by more than one
Review Comment:
The Javadoc has a typo at the beginning ("cWe"), which reads like an
accidental character and makes the comment harder to understand. Please fix it
(e.g., "We need to reclaim...").
```suggestion
* We need to reclaim resources from queries that have exceeded their
timeout by more than one
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/QueryExecution.java:
##########
@@ -395,6 +390,18 @@ private void releaseResource(Throwable t) {
}
}
+ /**
+ * clear up Coordinator.queryExecutionMap by calling
Coordinator.cleanupQueryExecution if the
+ * current rpc is finished. We need to make sure the cleanup logic is only
called when client
+ * connection is not active, otherwise the finally code logic in
ClientRPCServiceImpl will handle
+ * that
Review Comment:
This comment says cleanup should only run when the client connection is not
active, but the actual condition is `startTimeOfCurrentRpc == -1` (i.e., no RPC
in progress). Please either adjust the condition to reflect actual connection
activity, or update the comment to avoid implying a stronger guarantee than the
code provides.
```suggestion
* Clear up Coordinator.queryExecutionMap by calling
Coordinator.cleanupQueryExecution if there is
* no RPC in progress for this query (that is, the current RPC has
finished and
* {@code startTimeOfCurrentRpc == -1}). In cases where an RPC is still
active, the finally block
* in ClientRPCServiceImpl is responsible for performing the cleanup.
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/QueryExecution.java:
##########
@@ -104,9 +105,14 @@ public class QueryExecution implements IQueryExecution {
private final AtomicBoolean stopped;
- // cost time in ns
+ // cost time in ns of finished rpc
private long totalExecutionTime = 0;
+ // -1 if previous rpc is finished and next client req hasn't come yet, unit
is ns
+ // it will be updated in fetchResult rpc
+ // protected by synchronized(this)
Review Comment:
The comment says `startTimeOfCurrentRpc` should be `-1` when no RPC is in
progress, but the field is initialized to `System.nanoTime()`, which makes the
state ambiguous (and can include time before any RPC actually starts, depending
on construction timing). Consider initializing it to `-1` and explicitly
calling `updateCurrentRpcStartTime(...)` at the start of each relevant RPC, or
clarify in the comment that the initial value represents an in-progress RPC
from construction time.
```suggestion
// Start time in ns of the current RPC.
// Initialized at construction time and updated in fetchResult RPCs.
// Protected by synchronized(this).
```
--
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]
