Copilot commented on code in PR #151:
URL: https://github.com/apache/iotdb-client-go/pull/151#discussion_r2797108790
##########
client/utils.go:
##########
@@ -249,7 +249,7 @@ func verifySuccesses(statuses []*common.TSStatus) error {
}
errMsg := buff.String()
if len(errMsg) > 0 {
- return NewBatchError(statuses)
+ return &BatchError{statuses, errMsg}
Review Comment:
While refactoring `verifySuccesses` to return `BatchError`, the loop above
still does `buff.WriteString(*status.Message + ";")` without a nil check. If
any sub-status has `Message == nil`, this will panic when building the batch
error. Please guard against nil (and consider including the status code when
message is nil), and add a unit test for a nil `Message` sub-status.
##########
client/errors.go:
##########
@@ -20,29 +20,34 @@
package client
import (
- "bytes"
+ "fmt"
"github.com/apache/iotdb-client-go/v2/common"
)
+// ExecutionError represents an error returned by the server via TSStatus.
+// It is NOT a connection error and should not cause session drops.
+type ExecutionError struct {
+ Code int32
+ Message string
+}
+
+func (e *ExecutionError) Error() string {
+ if e.Message != "" {
+ return fmt.Sprintf("error code: %d, message: %v", e.Code,
e.Message)
+ }
+ return fmt.Sprintf("error code: %d", e.Code)
+}
+
type BatchError struct {
statuses []*common.TSStatus
+ Message string
}
Review Comment:
`ExecutionError.Error()` only includes the ", message: ..." part when
`Message != ""`. If the server sets `TSStatus.Message` to a non-nil but empty
string, the previous behavior (from `fmt.Errorf` in `VerifySuccess`) would
still include the message field, but this new type will omit it, changing the
error string. To preserve semantics and avoid surprising downstream
error-string comparisons, consider representing the message as `*string` (or
tracking a `HasMessage` flag) and formatting based on nil vs non-nil rather
than empty vs non-empty.
```suggestion
return fmt.Sprintf("error code: %d, message: %v", e.Code, e.Message)
}
type BatchError struct {
statuses []*common.TSStatus
Message string
}
statuses []*common.TSStatus
Message string
}
```
##########
client/tablesessionpool.go:
##########
@@ -75,23 +74,41 @@ type PooledTableSession struct {
closed int32
}
+// isConnectionError returns true if the error is a connection-level error
+// (i.e., not a server-side execution error indicated by TSStatus).
+func isConnectionError(err error) bool {
+ if err == nil {
+ return false
+ }
+ var exeErr *ExecutionError
+ if errors.As(err, &exeErr) {
+ return false
+ }
+ var batchErr *BatchError
+ if errors.As(err, &batchErr) {
+ return false
+ }
+ return true
+}
Review Comment:
`isConnectionError` currently treats any error that is not `*ExecutionError`
or `*BatchError` as a connection-level error. That includes client-side
validation/serialization errors (e.g., from `tablet.getValuesBytes()` /
`genTSInsertTabletReq`) and context cancellation/timeouts, which are not
necessarily reasons to drop and discard a healthy pooled session. Consider
flipping the logic to positively identify actual transport/connection failures
(e.g., thrift transport exceptions, `io.EOF`, `net.Error`,
`context.DeadlineExceeded` as desired) rather than assuming everything else is
a connection error.
--
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]
