Copilot commented on code in PR #17061:
URL: https://github.com/apache/iotdb/pull/17061#discussion_r2716162083
##########
iotdb-core/ainode/iotdb/ainode/core/rpc/handler.py:
##########
@@ -92,6 +92,11 @@ def loadModel(self, req: TLoadModelReq) -> TSStatus:
status = self._ensure_model_is_registered(req.existingModelId)
if status.code != TSStatusCode.SUCCESS_STATUS.value:
return status
+ if set(req.deviceIdList) != req.deviceIdList:
Review Comment:
The comparison `set(req.deviceIdList) != req.deviceIdList` is incorrect for
detecting duplicates in a Python list. In Python, comparing a set to a list
will always evaluate to True (not equal) even when there are no duplicates,
because they are different types. The correct approach is to compare the length
of the list with the length of the set: `len(set(req.deviceIdList)) !=
len(req.deviceIdList)`.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/udf/UDTFForecast.java:
##########
@@ -214,8 +215,7 @@ private TsBlock forecast() throws Exception {
new TForecastReq(model_id, serde.serialize(inputData),
outputLength)
.setOptions(options));
} catch (Exception e) {
- throw new IoTDBRuntimeException(
- e.getMessage(), TSStatusCode.CAN_NOT_CONNECT_AINODE.getStatusCode());
+ throw new AINodeConnectionException();
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
```suggestion
throw new AINodeConnectionException(e);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/function/tvf/ForecastTableFunction.java:
##########
@@ -564,7 +564,7 @@ protected TsBlock forecast() {
new TForecastReq(modelId, SERDE.serialize(inputData),
outputLength)
.setOptions(options));
} catch (Exception e) {
- throw new IoTDBRuntimeException(e.getMessage(),
CAN_NOT_CONNECT_AINODE.getStatusCode());
+ throw new AINodeConnectionException();
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
```suggestion
throw new AINodeConnectionException(e);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3706,7 +3707,7 @@ public SettableFuture<ConfigTaskResult>
createModel(String modelId, String uri)
future.set(new ConfigTaskResult(TSStatusCode.SUCCESS_STATUS));
}
} catch (final TException | ClientManagerException e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/function/tvf/ClassifyTableFunction.java:
##########
@@ -368,7 +368,7 @@ private TsBlock classify() {
CLIENT_MANAGER.borrowClient(AINodeClientManager.AINODE_ID_PLACEHOLDER)) {
resp = client.forecast(new TForecastReq(modelId,
SERDE.serialize(inputData), outputLength));
} catch (Exception e) {
- throw new IoTDBRuntimeException(e.getMessage(),
CAN_NOT_CONNECT_AINODE.getStatusCode());
+ throw new AINodeConnectionException();
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
```suggestion
throw new AINodeConnectionException(e);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3744,7 +3745,7 @@ public SettableFuture<ConfigTaskResult> showModels(final
String modelId) {
}
ShowModelsTask.buildTsBlock(resp, future);
} catch (final Exception e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3799,7 +3800,7 @@ public SettableFuture<ConfigTaskResult> loadModel(
future.set(new ConfigTaskResult(TSStatusCode.SUCCESS_STATUS));
}
} catch (final Exception e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3780,7 +3781,7 @@ public SettableFuture<ConfigTaskResult> showAIDevices() {
}
ShowAIDevicesTask.buildTsBlock(resp, future);
} catch (final Exception e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
##########
iotdb-core/ainode/iotdb/ainode/core/rpc/handler.py:
##########
@@ -104,6 +109,11 @@ def unloadModel(self, req: TUnloadModelReq) -> TSStatus:
status = self._ensure_model_is_registered(req.modelId)
if status.code != TSStatusCode.SUCCESS_STATUS.value:
return status
+ if set(req.deviceIdList) != req.deviceIdList:
Review Comment:
The comparison `set(req.deviceIdList) != req.deviceIdList` is incorrect for
detecting duplicates in a Python list. In Python, comparing a set to a list
will always evaluate to True (not equal) even when there are no duplicates,
because they are different types. The correct approach is to compare the length
of the list with the length of the set: `len(set(req.deviceIdList)) !=
len(req.deviceIdList)`.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/process/ai/InferenceOperator.java:
##########
@@ -248,7 +249,7 @@ private void submitInferenceTask() {
modelInferenceDescriptor.getModelId(),
serde.serialize(inputTsBlock))
.setInferenceAttributes(modelInferenceDescriptor.getInferenceAttributes()));
} catch (Exception e) {
- throw new ModelInferenceProcessException(e.getMessage());
+ throw new AINodeConnectionException();
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
```suggestion
throw new AINodeConnectionException(e);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3763,7 +3764,7 @@ public SettableFuture<ConfigTaskResult>
showLoadedModels(List<String> deviceIdLi
}
ShowLoadedModelsTask.buildTsBlock(resp, future);
} catch (final Exception e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3851,7 +3852,7 @@ public SettableFuture<ConfigTaskResult> createTuningTask(
future.set(new ConfigTaskResult(TSStatusCode.SUCCESS_STATUS));
}
} catch (final Exception e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3818,7 +3819,7 @@ public SettableFuture<ConfigTaskResult> unloadModel(
future.set(new ConfigTaskResult(TSStatusCode.SUCCESS_STATUS));
}
} catch (final Exception e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java:
##########
@@ -3723,7 +3724,7 @@ public SettableFuture<ConfigTaskResult> dropModel(final
String modelId) {
future.set(new ConfigTaskResult(TSStatusCode.SUCCESS_STATUS));
}
} catch (final ClientManagerException | TException e) {
- future.setException(e);
+ future.setException(new AINodeConnectionException());
Review Comment:
The new AINodeConnectionException discards the original exception cause,
which loses important debugging information such as the root cause of the
connection failure (e.g., network timeout, authentication failure, etc.).
Consider adding a constructor that accepts a Throwable cause parameter and pass
the original exception when creating the AINodeConnectionException, similar to
how IoTDBRuntimeException supports cause chaining.
--
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]
