fix(client): re-send session configuration on reconnect - #178
Open
taka-2628 wants to merge 1 commit into
Open
Conversation
Session.initClusterConn() built its TSOpenSessionReq without a Configuration map, while Open() and OpenCluster() populate Configuration["sql_dialect"], ["version"], and ["db"]. Because reconnect() routes exclusively through initClusterConn() and is the sole transparent-reconnect path, any session that reconnects after a server restart silently reverted to the server-default dialect with no database bound. For table-model sessions this poisons the session: every subsequent statement fails with a SqlParseError (700), and because that is a valid server response rather than a transport error, TableSessionPool keeps the session in the pool. Extract the Configuration-building block into a single helper and call it from all three open paths so the reconnect path sends what open sends and the three cannot drift again. Add a table-driven test for the helper. Verified against apache/iotdb:2.0.3-standalone: a table-model insert on a pooled session that reconnects after `docker restart` fails on the current code and succeeds with this change. Fixes apache#177 Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Session.initClusterConn()builds itsTSOpenSessionReqwithout aConfigurationmap, whileSession.Open()andSession.OpenCluster()both populateConfiguration["sql_dialect"],["version"], and["db"]. Sincereconnect()— the sole transparent-reconnect path, invoked from every insert/query/execute method on a transport error — routes exclusively throughinitClusterConn(), any session that reconnects after a server restart silently reverts to the server-default dialect with no database bound.For table-model sessions this poisons the session: every subsequent statement is rejected with a
SqlParseError(700) /SemanticError(701). Because these are valid server responses (*ExecutionError) rather than Go transport errors,TableSessionPool.isConnectionErrorreturns false for them and the poisoned session is returned to the pool until the process restarts. The reference Java client re-sends this configuration on reconnect (SessionConnection.init()viareconnect()); the Go client did not.Full analysis, including the diagnosis path and the pool-poisoning mechanism, in #177.
This bug exists because the same request-configuration block was hand-duplicated across three open paths and one copy was missed. This change extracts a single helper,
buildOpenSessionConfiguration, and wires all three sites (Open,OpenCluster,initClusterConn) through it so they cannot drift again.s.configalready carriessqlDialect/Databaseat reconnect time. Cluster sessions carry noVersion(ClusterConfighas no such field), so for them the empty-Version→DEFAULT_VERSIONfallback reproduces exactly whatOpenCluster()already sends.Verification
apache/iotdb:2.0.3-standalonewith aTableSessionPoolbound to a database: insert (succeeds) →docker restartthe server → insert again on the reconnected pooled session. On the current code the post-restart insert fails witherror code: 700 ... mismatched input 't' expecting ROOT(session reverted to tree dialect); with this change the post-restart insert succeeds. Same test, only the library changed.TestBuildOpenSessionConfiguration(table-driven, matching the existingsession_test.gostyle).go build ./...,go vet ./client/, andgo test ./client/pass.Out of scope (noted for maintainers)
initClusterConnalso never setss.timeFactorviagetTimeFactor(resp), whichOpen/OpenClusterdo — a reconnected session keeps a staletimeFactor. Benign on homogeneous clusters; happy to fold it into this PR if preferred.Fixes #177