Copilot commented on code in PR #151:
URL: https://github.com/apache/iotdb-client-go/pull/151#discussion_r2791214945
##########
client/session.go:
##########
@@ -739,7 +778,7 @@ func (s *Session) InsertRecordsOfOneDevice(deviceId string,
timestamps []int64,
ValuesList: valuesList,
}
- r, err = s.client.InsertRecordsOfOneDevice(context.Background(),
request)
+ r, err := s.client.InsertRecordsOfOneDevice(context.Background(),
request)
Review Comment:
`InsertRecordsOfOneDevice` introduces an outer `err` variable and later uses
`r, err := ...`, which shadows `err`. This makes the flow harder to follow and
is easy to trip with future edits (and can be flagged by linters). Consider
avoiding shadowing by scoping the loop error variable differently (or
restructuring the value conversion) so the RPC call doesn't redeclare `err`.
##########
client/tablesessionpool.go:
##########
@@ -81,17 +79,16 @@ type PooledTableSession struct {
// - tablet: A pointer to a Tablet containing time-series data to be
inserted.
//
// Returns:
-// - r: A pointer to TSStatus indicating the execution result.
// - err: An error if an issue occurs during the operation.
-func (s *PooledTableSession) Insert(tablet *Tablet) (r *common.TSStatus, err
error) {
- r, err = s.session.insertRelationalTablet(tablet)
+func (s *PooledTableSession) Insert(tablet *Tablet) error {
+ err := s.session.insertRelationalTablet(tablet)
if err == nil {
- return
+ return nil
}
s.sessionPool.dropSession(s.session)
atomic.StoreInt32(&s.closed, 1)
s.session = Session{}
- return
+ return err
Review Comment:
`PooledTableSession.Insert` now drops/invalidates the pooled session on any
returned error. Since `insertRelationalTablet` now calls `VerifySuccess`
internally, non-success server status codes (e.g., user/SQL/data errors) will
also cause the session to be dropped even though the connection is still
healthy. Consider only dropping the session for transport/connection errors, or
otherwise preserve a way to distinguish retryable/connection failures from
application-level status errors.
##########
client/session.go:
##########
@@ -457,11 +484,14 @@ func (s *Session) GetTimeZone() (string, error) {
return resp.TimeZone, nil
}
-func (s *Session) SetTimeZone(timeZone string) (r *common.TSStatus, err error)
{
+func (s *Session) SetTimeZone(timeZone string) error {
request := rpc.TSSetTimeZoneReq{SessionId: s.sessionId, TimeZone:
timeZone}
- r, err = s.client.SetTimeZone(context.Background(), &request)
+ r, err := s.client.SetTimeZone(context.Background(), &request)
s.config.TimeZone = timeZone
- return r, err
+ if err != nil {
+ return err
+ }
+ return VerifySuccess(r)
Review Comment:
`SetTimeZone` updates `s.config.TimeZone` before checking the RPC error /
`VerifySuccess`. If the call fails (transport error or non-success status), the
Session will keep an incorrect local timezone. Move the assignment to after
`VerifySuccess(r)` succeeds (or only update when returning nil).
##########
client/session.go:
##########
@@ -782,7 +825,7 @@ func (s *Session) InsertAlignedRecordsOfOneDevice(deviceId
string, timestamps []
IsAligned: &isAligned,
}
- r, err = s.client.InsertRecordsOfOneDevice(context.Background(),
request)
+ r, err := s.client.InsertRecordsOfOneDevice(context.Background(),
request)
Review Comment:
`InsertAlignedRecordsOfOneDevice` has the same `err` shadowing pattern (`var
err error` then `r, err := ...`). This is unnecessarily confusing and can lead
to subtle mistakes; consider restructuring to avoid redeclaring `err` at the
RPC call site.
```suggestion
var r *rpc.TSStatus
r, err = s.client.InsertRecordsOfOneDevice(context.Background(),
request)
```
##########
client/tablesessionpool.go:
##########
@@ -100,17 +97,16 @@ func (s *PooledTableSession) Insert(tablet *Tablet) (r
*common.TSStatus, err err
// - sql: The SQL statement to execute.
//
// Returns:
-// - r: A pointer to TSStatus indicating the execution result.
// - err: An error if an issue occurs during the operation.
-func (s *PooledTableSession) ExecuteNonQueryStatement(sql string) (r
*common.TSStatus, err error) {
- r, err = s.session.ExecuteNonQueryStatement(sql)
+func (s *PooledTableSession) ExecuteNonQueryStatement(sql string) error {
+ err := s.session.ExecuteNonQueryStatement(sql)
if err == nil {
- return
+ return nil
}
s.sessionPool.dropSession(s.session)
atomic.StoreInt32(&s.closed, 1)
s.session = Session{}
- return
+ return err
Review Comment:
`PooledTableSession.ExecuteNonQueryStatement` drops the underlying session
for any error returned by `Session.ExecuteNonQueryStatement`. After this
refactor, that error can come from `VerifySuccess` (i.e., a non-success
TSStatus) and not necessarily a broken connection. Dropping the session for
application-level errors can cause unnecessary churn in the pool; consider only
dropping on connection/transport failures.
--
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]
