[5/5] feat(bigtable): add Session lifecycle, vRPC, and tests - #20112
Open
sushanb wants to merge 4 commits into
Open
[5/5] feat(bigtable): add Session lifecycle, vRPC, and tests#20112sushanb wants to merge 4 commits into
sushanb wants to merge 4 commits into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive transport and session management layer for Bigtable vRPCs, including session lifecycle management, heartbeat monitoring, and detailed observability/metrics tracking. The review feedback highlights several critical areas for improvement, primarily focusing on preventing potential nil pointer dereferences when processing server responses (such as error responses, session parameters, and vRPC responses). Additionally, the feedback suggests moving the OnStart hook to guarantee correct lifecycle ordering during early initialization failures, and correcting the error classification of stream send failures to StateUncommitted to ensure non-idempotent operations can be safely retried.
Comment on lines
+292
to
+293
| func (s *Session) handleErrorResponse(errResp *spb.ErrorResponse) { | ||
| if errResp.GetRpcId() != 0 { |
Contributor
There was a problem hiding this comment.
If errResp is nil, accessing errResp.Status directly on line 298 will cause a nil pointer dereference panic. Add a nil check at the beginning of handleErrorResponse to safely guard against nil payloads.
func (s *Session) handleErrorResponse(errResp *spb.ErrorResponse) {
if errResp == nil {
return
}
if errResp.GetRpcId() != 0 {
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — added if errResp == nil { return } at the top of handleErrorResponse.
Comment on lines
+308
to
+311
| // handleSessionParameters updates the heartbeat interval negotiated by the | ||
| // server and immediately recomputes the watchdog deadline against the new | ||
| // interval. | ||
| func (s *Session) handleSessionParameters(params *spb.SessionParametersResponse) { |
Contributor
There was a problem hiding this comment.
If the server sends a SessionResponse with a nil SessionParameters payload (or if it is nil in test/fake environments), accessing params.KeepAlive directly will cause a nil pointer dereference panic. Add a nil check for params before accessing its fields.
Suggested change
| // handleSessionParameters updates the heartbeat interval negotiated by the | |
| // server and immediately recomputes the watchdog deadline against the new | |
| // interval. | |
| func (s *Session) handleSessionParameters(params *spb.SessionParametersResponse) { | |
| func (s *Session) handleSessionParameters(params *spb.SessionParametersResponse) { | |
| if params == nil || params.KeepAlive == nil { | |
| return | |
| } |
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — first line is now if params == nil || params.KeepAlive == nil { return }.
Comment on lines
+224
to
+227
| if res.errResp != nil { | ||
| return tagErr(StateServerResult, errorResponseToErr(res.errResp)) | ||
| } | ||
| if res.resp.RpcId != rpc.id { |
Contributor
There was a problem hiding this comment.
If res.resp is nil (e.g., due to a zero-initialized vrpcResult or an unexpected channel closure), accessing res.resp.RpcId directly will cause a nil pointer dereference panic. Add a defensive nil check for res.resp before accessing its fields.
Suggested change
| if res.errResp != nil { | |
| return tagErr(StateServerResult, errorResponseToErr(res.errResp)) | |
| } | |
| if res.resp.RpcId != rpc.id { | |
| if res.errResp != nil { | |
| return tagErr(StateServerResult, errorResponseToErr(res.errResp)) | |
| } | |
| if res.resp == nil { | |
| return tagErr(StateServerResult, fmt.Errorf("internal: received nil vRPC response")) | |
| } | |
| if res.resp.RpcId != rpc.id { |
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — added an if res.resp == nil { return tagErr(StateServerResult, …) } branch before the RpcId check. Kept the classification at StateServerResult (bookkeeping invariant, not a transport event) and dropped the debug-tag emission so no new constant is needed in the already-open #20114.
Comment on lines
+268
to
+269
| func (s *Session) handleVRPCResponse(resp *spb.VirtualRpcResponse) { | ||
| // A vRPC response is only expected while the session is Ready or |
Contributor
There was a problem hiding this comment.
If resp is nil, accessing resp.RpcId directly on line 274 will cause a nil pointer dereference panic. Add a nil check at the beginning of handleVRPCResponse to safely guard against nil payloads.
Suggested change
| func (s *Session) handleVRPCResponse(resp *spb.VirtualRpcResponse) { | |
| // A vRPC response is only expected while the session is Ready or | |
| func (s *Session) handleVRPCResponse(resp *spb.VirtualRpcResponse) { | |
| if resp == nil { | |
| return | |
| } | |
| // A vRPC response is only expected while the session is Ready or |
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — nil-guard is the first statement in handleVRPCResponse.
Comment on lines
+296
to
+297
| func (s *Session) handleVRPCErrorResponse(errResp *spb.ErrorResponse) { | ||
| st := s.State() |
Contributor
There was a problem hiding this comment.
If errResp is nil, accessing errResp.RpcId directly on line 299 will cause a nil pointer dereference panic. Add a nil check at the beginning of handleVRPCErrorResponse to safely guard against nil payloads.
func (s *Session) handleVRPCErrorResponse(errResp *spb.ErrorResponse) {
if errResp == nil {
return
}
st := s.State()
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — nil-guard is the first statement in handleVRPCErrorResponse.
Comment on lines
+38
to
+45
| func (s *Session) Start(ctx context.Context, req *spb.OpenSessionRequest) error { | ||
| if prev, ok := s.transitionTo(StateStarting, isState(StateNew)); !ok { | ||
| return fmt.Errorf("session already started or closed (state: %v)", prev) | ||
| } | ||
|
|
||
| openReq := &spb.SessionRequest{ | ||
| Payload: &spb.SessionRequest_OpenSession{OpenSession: req}, | ||
| } |
Contributor
There was a problem hiding this comment.
To ensure the lifecycle ordering guarantee that OnStart always precedes OnClose, the onStart hook should be fired immediately after transitioning to StateStarting. Currently, if s.Send(openReq) fails, s.ForceClose is called (which triggers OnClose), but onStart is never fired because it is placed at the end of the function.
func (s *Session) Start(ctx context.Context, req *spb.OpenSessionRequest) error {
if prev, ok := s.transitionTo(StateStarting, isState(StateNew)); !ok {
return fmt.Errorf("session already started or closed (state: %v)", prev)
}
s.hooks.onStart(ctx)
openReq := &spb.SessionRequest{
Payload: &spb.SessionRequest_OpenSession{OpenSession: req},
}
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — s.hooks.onStart(ctx) now fires immediately after transitionTo(StateStarting), before Send. Comment on the line explains the ordering invariant.
Comment on lines
+54
to
+58
| go s.readLoop(ctx) | ||
| go s.heartBeatLoop(ctx) | ||
|
|
||
| s.hooks.onStart(ctx) | ||
| return nil |
Contributor
There was a problem hiding this comment.
Remove the onStart hook call from here as it has been moved to the beginning of the Start function to guarantee correct lifecycle ordering even on early initialization failures.
Suggested change
| go s.readLoop(ctx) | |
| go s.heartBeatLoop(ctx) | |
| s.hooks.onStart(ctx) | |
| return nil | |
| go s.readLoop(ctx) | |
| go s.heartBeatLoop(ctx) | |
| return nil |
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — removed; onStart moved to the top of Start (see thread above).
Comment on lines
+128
to
+130
| if err := s.Send(sessionReq); err != nil { | ||
| return result, tagErr(StateTransportFailure, fmt.Errorf("send vRPC request: %w", err)) | ||
| } |
Contributor
There was a problem hiding this comment.
If s.Send fails, the request failed to write to the stream and never reached the wire. Tagging this error as StateTransportFailure is overly conservative and prevents unconditional retries for non-idempotent operations. It should be tagged as StateUncommitted instead.
Suggested change
| if err := s.Send(sessionReq); err != nil { | |
| return result, tagErr(StateTransportFailure, fmt.Errorf("send vRPC request: %w", err)) | |
| } | |
| if err := s.Send(sessionReq); err != nil { | |
| return result, tagErr(StateUncommitted, fmt.Errorf("send vRPC request: %w", err)) | |
| } |
Contributor
Author
There was a problem hiding this comment.
Done in c30e82c94d — retagged as StateUncommitted with a comment citing Java parity (java-bigtable classifies pre-wire Send errors as Uncommitted; the frame never left the client).
sushanb
force-pushed
the
bigtable-session-core
branch
from
July 8, 2026 23:45
156b9e1 to
64e5d43
Compare
This was referenced Jul 8, 2026
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
Adds the Session struct itself along with the two "surface" halves that have no lifecycle behavior of their own — the OTel tracer and the in-process debug counters / event ring. Also adds SessionHandle, the per-session slot the pool checkout / picker code holds. Lifecycle (Start / Close / readLoop / heartBeatLoop) and vRPC (Invoke) come next in the reshaped googleapis#20112. - session.go — Session struct + hooks + NewSession + State transitions (transitionTo, is/notState, signalQuiescent), vrpcResult, sessionErr, unavailable, afeID. Uses the existing State enum from session_state.go (PR googleapis#19981) — no re-declaration. - session_debug.go — embedded sessionDebug: counters (retries, okRpcs, errorRpcs, msgsSent, msgsRecv), per-session event ring, latency histogram, cluster-id map, WithSessionLogger / WithSessionPoolName options, RemoteAddr / SampleUptime / RecordTransportOverhead accessors. Consumes metrics.TransportTypeName (googleapis#20115) and recordDebugTag (googleapis#20114). - session_tracer.go — OTel sessionTracer for per-session + per-attempt metrics (session duration, open latency, uptime, transport-overhead histogram). Consumes metrics.TransportTypeName. - picker.go — SessionHandle wrapping *Session with the per-pick counters (Picks, Outstanding, LastActivity) the pool needs. Its concrete users (SessionPool, AFE picker) come in later PRs; the handle type is here because Session embeds an atomic.Pointer to it. Part 3b/3c of the Session core sub-split. Stacks on bigtable-session-primitives (Part 3a), googleapis#20115 (metrics.TransportTypeName export), and googleapis#20114 (debug tag counter).
5 tasks
sushanb
force-pushed
the
bigtable-session-core
branch
from
July 9, 2026 00:06
64e5d43 to
c30e82c
Compare
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
…type) Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - attempt_outcome.go — AttemptState (StateUncommitted / StateTransportFailure / StateServerResult), tagErr, TagErr, ClassifyErr. Models Java's VRpc.VRpcResult.State so the RetryingVRpc interceptor (later PR) can classify errors the same way as java-bigtable. - vrpc.go — ctx-metadata helpers (WithVRpcMetadata, WithAttempt, VRpcAttempt, VRpcMethod, WithPrevAttemptErr, PrevAttemptErr). Session Invoke reads these from its ctx. - session_msgtype.go — reqMsgType / respMsgType enums + classifyReq / classifyResp helpers. Used by the debug surface + tracer to bucket Session request/response types. Part 3a/3c of the Session core sub-split (a follow-up to the original Session core PR googleapis#20112, which is being reshaped into three thinner PRs). Stacks on googleapis#20115 (metrics.TransportTypeName export) and googleapis#20114 (debug tag counter); each of those can merge in any order.
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
Adds the Session struct itself along with the two "surface" halves that have no lifecycle behavior of their own — the OTel tracer and the in-process debug counters / event ring. Also adds SessionHandle, the per-session slot the pool checkout / picker code holds. Lifecycle (Start / Close / readLoop / heartBeatLoop) and vRPC (Invoke) come next in the reshaped googleapis#20112. - session.go — Session struct + hooks + NewSession + State transitions (transitionTo, is/notState, signalQuiescent), vrpcResult, sessionErr, unavailable, afeID. Uses the existing State enum from session_state.go (PR googleapis#19981) — no re-declaration. - session_debug.go — embedded sessionDebug: counters (retries, okRpcs, errorRpcs, msgsSent, msgsRecv), per-session event ring, latency histogram, cluster-id map, WithSessionLogger / WithSessionPoolName options, RemoteAddr / SampleUptime / RecordTransportOverhead accessors. Consumes metrics.TransportTypeName (googleapis#20115) and recordDebugTag (googleapis#20114). - session_tracer.go — OTel sessionTracer for per-session + per-attempt metrics (session duration, open latency, uptime, transport-overhead histogram). Consumes metrics.TransportTypeName. - picker.go — SessionHandle wrapping *Session with the per-pick counters (Picks, Outstanding, LastActivity) the pool needs. Its concrete users (SessionPool, AFE picker) come in later PRs; the handle type is here because Session embeds an atomic.Pointer to it. Part 3b/3c of the Session core sub-split. Stacks on bigtable-session-primitives (Part 3a), googleapis#20115 (metrics.TransportTypeName export), and googleapis#20114 (debug tag counter).
sushanb
force-pushed
the
bigtable-session-core
branch
from
July 9, 2026 00:17
c30e82c to
4c900a7
Compare
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
…type) Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - attempt_outcome.go — AttemptState (StateUncommitted / StateTransportFailure / StateServerResult), tagErr, TagErr, ClassifyErr. Models Java's VRpc.VRpcResult.State so the RetryingVRpc interceptor (later PR) can classify errors the same way as java-bigtable. - vrpc.go — ctx-metadata helpers (WithVRpcMetadata, WithAttempt, VRpcAttempt, VRpcMethod, WithPrevAttemptErr, PrevAttemptErr). Session Invoke reads these from its ctx. - session_msgtype.go — reqMsgType / respMsgType enums + classifyReq / classifyResp helpers. Used by the debug surface + tracer to bucket Session request/response types. Part 3a/3c of the Session core sub-split (a follow-up to the original Session core PR googleapis#20112, which is being reshaped into three thinner PRs). Stacks on googleapis#20115 (metrics.TransportTypeName export) and googleapis#20114 (debug tag counter); each of those can merge in any order.
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
Adds the Session struct itself along with the two "surface" halves that have no lifecycle behavior of their own — the OTel tracer and the in-process debug counters / event ring. Also adds SessionHandle, the per-session slot the pool checkout / picker code holds. Lifecycle (Start / Close / readLoop / heartBeatLoop) and vRPC (Invoke) come next in the reshaped googleapis#20112. - session.go — Session struct + hooks + NewSession + State transitions (transitionTo, is/notState, signalQuiescent), vrpcResult, sessionErr, unavailable, afeID. Uses the existing State enum from session_state.go (PR googleapis#19981) — no re-declaration. - session_debug.go — embedded sessionDebug: counters (retries, okRpcs, errorRpcs, msgsSent, msgsRecv), per-session event ring, latency histogram, cluster-id map, WithSessionLogger / WithSessionPoolName options, RemoteAddr / SampleUptime / RecordTransportOverhead accessors. Consumes metrics.TransportTypeName (googleapis#20115) and recordDebugTag (googleapis#20114). - session_tracer.go — OTel sessionTracer for per-session + per-attempt metrics (session duration, open latency, uptime, transport-overhead histogram). Consumes metrics.TransportTypeName. - picker.go — SessionHandle wrapping *Session with the per-pick counters (Picks, Outstanding, LastActivity) the pool needs. Its concrete users (SessionPool, AFE picker) come in later PRs; the handle type is here because Session embeds an atomic.Pointer to it. Part 3b/3c of the Session core sub-split. Stacks on bigtable-session-primitives (Part 3a), googleapis#20115 (metrics.TransportTypeName export), and googleapis#20114 (debug tag counter).
sushanb
force-pushed
the
bigtable-session-core
branch
from
July 9, 2026 00:19
4c900a7 to
53906d1
Compare
sushanb
added a commit
that referenced
this pull request
Jul 9, 2026
…g) (#20114) ## Summary Adds the transport-package debug-tag counter used at "this branch shouldn't reach" sites in the Session, session pool, and configuration manager. Every emission is one atomic add plus one OTel `Int64Counter` increment; safe to sprinkle freely on cold paths. Metric name (`debug_tags`) matches java-bigtable's `ClientDebugTagCount` so cross-language dashboards join on the tag column. Provides: - `recordDebugTag(name)` — cheap observation counter (Warn level). - `recordDebugTagAt(level, name)` — same, with an explicit level. - `assertDebugTag(expr, name)` / `assertDebugTagf` — invariant checks that increment the counter and log at Error level when they fail. - `DebugTags()` + `snapshotDebugTagCounts()` — read-side for debug pages. - Tag catalog constants (`tagSession*`, `tagVRPC*`, etc.) referenced from Session-lifecycle and vRPC code in the follow-up PR. Standalone — the Session/vRPC code that emits these tags lands in the Session core PR (#20112) stacked on top. Independent of #20115 (metrics `TransportTypeName` export); the two can merge in any order. **Part 2/3 of the Session core split.** ## Test plan - [x] `go build ./bigtable/...` - [x] `go vet ./bigtable/internal/transport/...` - [ ] CI: presubmit (dedicated `debug_tracer_test.go` lands in a follow-up along with a broader test-only split)
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
…type) Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - attempt_outcome.go — AttemptState (StateUncommitted / StateTransportFailure / StateServerResult), tagErr, TagErr, ClassifyErr. Models Java's VRpc.VRpcResult.State so the RetryingVRpc interceptor (later PR) can classify errors the same way as java-bigtable. - vrpc.go — ctx-metadata helpers (WithVRpcMetadata, WithAttempt, VRpcAttempt, VRpcMethod, WithPrevAttemptErr, PrevAttemptErr). Session Invoke reads these from its ctx. - session_msgtype.go — reqMsgType / respMsgType enums + classifyReq / classifyResp helpers. Used by the debug surface + tracer to bucket Session request/response types. Part 3a/3c of the Session core sub-split (a follow-up to the original Session core PR googleapis#20112, which is being reshaped into three thinner PRs). Stacks on googleapis#20115 (metrics.TransportTypeName export) and googleapis#20114 (debug tag counter); each of those can merge in any order.
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
…type) Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - attempt_outcome.go — AttemptState (StateUncommitted / StateTransportFailure / StateServerResult), tagErr, TagErr, ClassifyErr. Models Java's VRpc.VRpcResult.State so the RetryingVRpc interceptor (later PR) can classify errors the same way as java-bigtable. - vrpc.go — ctx-metadata helpers (WithVRpcMetadata, WithAttempt, VRpcAttempt, VRpcMethod, WithPrevAttemptErr, PrevAttemptErr). Session Invoke reads these from its ctx. - session_msgtype.go — reqMsgType / respMsgType enums + classifyReq / classifyResp helpers. Used by the debug surface + tracer to bucket Session request/response types. Part 3a/3c of the Session core sub-split (a follow-up to the original Session core PR googleapis#20112, which is being reshaped into three thinner PRs). Stacks on googleapis#20115 (metrics.TransportTypeName export) and googleapis#20114 (debug tag counter); each of those can merge in any order.
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
Adds the Session struct itself along with the two "surface" halves that have no lifecycle behavior of their own — the OTel tracer and the in-process debug counters / event ring. Also adds SessionHandle, the per-session slot the pool checkout / picker code holds. Lifecycle (Start / Close / readLoop / heartBeatLoop) and vRPC (Invoke) come next in the reshaped googleapis#20112. - session.go — Session struct + hooks + NewSession + State transitions (transitionTo, is/notState, signalQuiescent), vrpcResult, sessionErr, unavailable, afeID. Uses the existing State enum from session_state.go (PR googleapis#19981) — no re-declaration. - session_debug.go — embedded sessionDebug: counters (retries, okRpcs, errorRpcs, msgsSent, msgsRecv), per-session event ring, latency histogram, cluster-id map, WithSessionLogger / WithSessionPoolName options, RemoteAddr / SampleUptime / RecordTransportOverhead accessors. Consumes metrics.TransportTypeName (googleapis#20115) and recordDebugTag (googleapis#20114). - session_tracer.go — OTel sessionTracer for per-session + per-attempt metrics (session duration, open latency, uptime, transport-overhead histogram). Consumes metrics.TransportTypeName. - picker.go — SessionHandle wrapping *Session with the per-pick counters (Picks, Outstanding, LastActivity) the pool needs. Its concrete users (SessionPool, AFE picker) come in later PRs; the handle type is here because Session embeds an atomic.Pointer to it. Part 3b/3c of the Session core sub-split. Stacks on bigtable-session-primitives (Part 3a), googleapis#20115 (metrics.TransportTypeName export), and googleapis#20114 (debug tag counter).
Renames transportTypeName -> TransportTypeName in
bigtable/internal/metrics/util.go and updates its one caller in
tracer.go. Zero behavior change.
Motivation: the upcoming Session core PR needs the same
PeerInfo.TransportType -> short-label mapping ("cloudpath",
"session_directpath", ...) in the transport package's session tracer,
session_debug, and session_lifecycle. Exporting the existing helper is
strictly cheaper than duplicating the 15-line switch in a second
package. No import cycle (metrics does not import transport, and
transport does not currently import metrics).
Part 1/3 of the Session core PR split.
…type) Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - attempt_outcome.go — AttemptState (StateUncommitted / StateTransportFailure / StateServerResult), tagErr, TagErr, ClassifyErr. Models Java's VRpc.VRpcResult.State so the RetryingVRpc interceptor (later PR) can classify errors the same way as java-bigtable. - vrpc.go — ctx-metadata helpers (WithVRpcMetadata, WithAttempt, VRpcAttempt, VRpcMethod, WithPrevAttemptErr, PrevAttemptErr). Session Invoke reads these from its ctx. - session_msgtype.go — reqMsgType / respMsgType enums + classifyReq / classifyResp helpers. Used by the debug surface + tracer to bucket Session request/response types. Part 3a/3c of the Session core sub-split (a follow-up to the original Session core PR googleapis#20112, which is being reshaped into three thinner PRs). Stacks on googleapis#20115 (metrics.TransportTypeName export) and googleapis#20114 (debug tag counter); each of those can merge in any order.
Adds the Session struct itself along with the two "surface" halves that have no lifecycle behavior of their own — the OTel tracer and the in-process debug counters / event ring. Also adds SessionHandle, the per-session slot the pool checkout / picker code holds. Lifecycle (Start / Close / readLoop / heartBeatLoop) and vRPC (Invoke) come next in the reshaped googleapis#20112. - session.go — Session struct + hooks + NewSession + State transitions (transitionTo, is/notState, signalQuiescent), vrpcResult, sessionErr, unavailable, afeID. Uses the existing State enum from session_state.go (PR googleapis#19981) — no re-declaration. - session_debug.go — embedded sessionDebug: counters (retries, okRpcs, errorRpcs, msgsSent, msgsRecv), per-session event ring, latency histogram, cluster-id map, WithSessionLogger / WithSessionPoolName options, RemoteAddr / SampleUptime / RecordTransportOverhead accessors. Consumes metrics.TransportTypeName (googleapis#20115) and recordDebugTag (googleapis#20114). - session_tracer.go — OTel sessionTracer for per-session + per-attempt metrics (session duration, open latency, uptime, transport-overhead histogram). Consumes metrics.TransportTypeName. - picker.go — SessionHandle wrapping *Session with the per-pick counters (Picks, Outstanding, LastActivity) the pool needs. Its concrete users (SessionPool, AFE picker) come in later PRs; the handle type is here because Session embeds an atomic.Pointer to it. Part 3b/3c of the Session core sub-split. Stacks on bigtable-session-primitives (Part 3a), googleapis#20115 (metrics.TransportTypeName export), and googleapis#20114 (debug tag counter).
Adds the behavior half of the Session — the lifecycle state machine, the vRPC dispatch path, and the fake-driven test suite that exercises both. Together with the struct + surface half (bigtable-session-struct) and the primitives (bigtable-session-primitives), this completes the Session core. - session_lifecycle.go — Start, ForceClose, Close, Send, readLoop, heartBeatLoop, handleOpenSession / handleErrorResponse / handleGoAway / handleClose / handleSessionParameters, peerInfoExtracter. - session_vrpc.go — Invoke, buildInvokeRequest, awaitInvokeResult, handleVRPCResponse / handleVRPCErrorResponse, deliver, cancelActiveRPCs, noteRetryAttempt, releaseSlot. - session_test.go — fakeStream / fakeDesc plus tests for handleOpenSession, handleVRPC*, handleGoAway, handleErrorResponse, Invoke, ForceClose, Close, heartBeatLoop, PeerInfo extraction, AfeID, Start. Also folds in the pending gemini-code-assist review findings so this PR merges without a follow-up: - Fire onStart immediately after transitionTo(StateStarting) so the "onStart precedes onClose" invariant holds even when Send fails and ForceClose fires onClose. - Nil-guard handleErrorResponse / handleSessionParameters / handleVRPCResponse / handleVRPCErrorResponse and the res.resp branch in awaitInvokeResult; drop instead of panicking in the readLoop goroutine. - Retag pre-wire Send failures in Invoke as StateUncommitted (was StateTransportFailure) so the retry interceptor can retry non-idempotent ops when the frame never reached the server — Java parity with java-bigtable's classification. Part 3c of 3 in the Session core sub-split. Stacks on bigtable-session-struct (Part 3b), bigtable-session-primitives (Part 3a), googleapis#20115 (metrics.TransportTypeName), and googleapis#20114 (debug tag counter).
sushanb
force-pushed
the
bigtable-session-core
branch
from
July 9, 2026 20:17
53906d1 to
b54b32f
Compare
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 9, 2026
…type) Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - attempt_outcome.go — AttemptState (StateUncommitted / StateTransportFailure / StateServerResult), tagErr, TagErr, ClassifyErr. Models Java's VRpc.VRpcResult.State so the RetryingVRpc interceptor (later PR) can classify errors the same way as java-bigtable. - vrpc.go — ctx-metadata helpers (WithVRpcMetadata, WithAttempt, VRpcAttempt, VRpcMethod, WithPrevAttemptErr, PrevAttemptErr). Session Invoke reads these from its ctx. - session_msgtype.go — reqMsgType / respMsgType enums + classifyReq / classifyResp helpers. Used by the debug surface + tracer to bucket Session request/response types. Part 3a/3c of the Session core sub-split (a follow-up to the original Session core PR googleapis#20112, which is being reshaped into three thinner PRs). Stacks on googleapis#20115 (metrics.TransportTypeName export) and googleapis#20114 (debug tag counter); each of those can merge in any order.
sushanb
added a commit
that referenced
this pull request
Jul 10, 2026
…type) (#20116) ## Summary Adds the standalone types that the Session struct and its lifecycle / vRPC / debug halves all depend on. Each file is self-contained (no cross-references) so this PR compiles and passes tests on its own. - **`attempt_outcome.go`** — `AttemptState` (`StateUncommitted` / `StateTransportFailure` / `StateServerResult`), `tagErr`, `TagErr`, `ClassifyErr`. Models Java's `VRpc.VRpcResult.State` so the `RetryingVRpc` interceptor (later PR) can classify errors the same way as java-bigtable. - **`vrpc.go`** — ctx-metadata helpers (`WithVRpcMetadata`, `WithAttempt`, `VRpcAttempt`, `VRpcMethod`, `WithPrevAttemptErr`, `PrevAttemptErr`). `Session.Invoke` reads these from its ctx. - **`session_msgtype.go`** — `reqMsgType` / `respMsgType` enums + `classifyReq` / `classifyResp` helpers. Used by the debug surface + tracer to bucket Session request/response types. **Part 3a of 3 in the Session core sub-split** (a follow-up to the original Session core PR #20112, which is being reshaped into three thinner PRs). Stacks on #20115 (`metrics.TransportTypeName` export) and #20114 (debug tag counter); each of those can merge in any order. Sub-split order: 1. **3a — this PR:** Session primitives (~337 LOC). 2. **3b — TBD:** Session struct + tracer + debug surface + picker (~1.1k LOC). 3. **3c — reshaped #20112:** Session lifecycle + vRPC + tests (~2k LOC). ## Test plan - [x] `go build ./bigtable/...` - [x] `go test ./bigtable/internal/transport/ -count=1 -short` — passes (4.0s) - [x] `gofmt -l bigtable/internal/transport/` — clean - [ ] CI: presubmit
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 10, 2026
Adds the Session struct itself along with the two "surface" halves that have no lifecycle behavior of their own — the OTel tracer and the in-process debug counters / event ring. Also adds SessionHandle, the per-session slot the pool checkout / picker code holds. Lifecycle (Start / Close / readLoop / heartBeatLoop) and vRPC (Invoke) come next in the reshaped googleapis#20112. - session.go — Session struct + hooks + NewSession + State transitions (transitionTo, is/notState, signalQuiescent), vrpcResult, sessionErr, unavailable, afeID. Uses the existing State enum from session_state.go (PR googleapis#19981) — no re-declaration. - session_debug.go — embedded sessionDebug: counters (retries, okRpcs, errorRpcs, msgsSent, msgsRecv), per-session event ring, latency histogram, cluster-id map, WithSessionLogger / WithSessionPoolName options, RemoteAddr / SampleUptime / RecordTransportOverhead accessors. Consumes metrics.TransportTypeName (googleapis#20115) and recordDebugTag (googleapis#20114). - session_tracer.go — OTel sessionTracer for per-session + per-attempt metrics (session duration, open latency, uptime, transport-overhead histogram). Consumes metrics.TransportTypeName. - picker.go — SessionHandle wrapping *Session with the per-pick counters (Picks, Outstanding, LastActivity) the pool needs. Its concrete users (SessionPool, AFE picker) come in later PRs; the handle type is here because Session embeds an atomic.Pointer to it. Part 3b/3c of the Session core sub-split. Stacks on bigtable-session-primitives (Part 3a), googleapis#20115 (metrics.TransportTypeName export), and googleapis#20114 (debug tag counter).
sushanb
added a commit
to sushanb/google-cloud-go
that referenced
this pull request
Jul 10, 2026
…onHandle Adds unit coverage for the four files introduced in this PR. The suite avoids lifecycle paths (Start / Close / readLoop / heartBeatLoop / Invoke) — those live in the reshaped googleapis#20112 — and instead pins the static / structural behavior added here. session_handle_test.go - Defaults (zero counters, zero lastActivity, createdAt preserved). - Inc/DecOutstanding round trip; Dec stamps GetLastActivity to now. - Picks() reads through the typed atomic. - Concurrent Inc+Dec balance under -race (regression guard for the 32-bit alignment fix that switched to atomic.Int64). session_test.go - NewSession defaults: StateNew, quiescent open, heartbeat interval seeded, embedded sessionDebug initialized, ChannelIndex -1, lastStateChangeNano non-zero. - WithSessionLogger / WithSessionPoolName wiring. - transitionTo: happy path stamps + advances state; predicate-reject path leaves state alone; racing New->Starting proves exactly one goroutine wins (CAS loop). - isState / notState predicate builders. - signalQuiescent closes chan + is idempotent under concurrent callers. - AfeID: zero pre-PeerInfo, correct after Store. - vrpcResult.ClusterInfo: three sources (resp / errResp / err). - unavailable() carries codes.Unavailable, unwraps to the sentinel, preserves the formatted detail message. - SessionHooks dispatchers are nil-safe and fire once per call. session_debug_test.go - recordEvent ordering + ring overflow at maxSessionEvents (=64). - snapshotEvents returns an independent copy. - recordLatency drops non-positive, grows to latencyWindow then wraps overwriting oldest. - snapshotLatencies returns sorted ascending. - percentile: empty / p<=0 / p>=100 edges + interior nearest-rank on a hand-computed 10-element slice. - recordCluster: empty id dropped, Load fast path vs LoadOrStore first sighting both increment, concurrent 100x50 fanout across 4 ids sums to the exact expected total. - ChannelIndex Set/Get. - setCloseReason: first non-empty wins; empty call records tagSessionCloseNoReason and leaves CloseReason() "". - RemoteAddr default empty + after Store. - peerInfoSummary "peer=unknown" fallback + full-populated line contains afe, region, subzone, gfe, transport_type. - Msg / RPC accessor round-trips. session_tracer_test.go - newSessionTracer defaults (startTime stamped, sessionType stored, openedAt zero, snapshot returns unknown transport). - setPoolName + setPeerInfo round-trip through snapshot(), with transportType derived via metrics.TransportTypeName. - recordOpen stamps openedAt even when histograms are unregistered. - vrpcCloseState full 2x2 truth table (none / all_ok / all_error / some_ok). - msSince positive after real elapsed. - End-to-end emission: InitializeSessionMetrics with a ManualReader, drive recordOpen -> sampleUptime -> recordTransportOverhead -> recordClose, and verify every emitted histogram carries the label set the metrics spec pins (transport_type, session_type, afe_location, session_name, plus method / closing_reason / vrpcs on the metrics that use them). Test plan - go build ./... — clean. - go test ./internal/transport/ -count=1 -short — pass (~4s). - go test -race ./internal/transport/ -count=1 -short -timeout=180s — pass (~5s). - gofmt -l ./internal/transport/session_*_test.go — clean.
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
1 participant
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.
Summary
Adds the behavior half of the Session — the lifecycle state machine, the vRPC dispatch path, and the fake-driven test suite that exercises both. Together with the struct + surface half (#20117) and the primitives (#20116), this completes the Session core.
session_lifecycle.go—Start,ForceClose,Close,Send,readLoop,heartBeatLoop,handleOpenSession/handleErrorResponse/handleGoAway/handleClose/handleSessionParameters,peerInfoExtracter.session_vrpc.go—Invoke,buildInvokeRequest,awaitInvokeResult,handleVRPCResponse/handleVRPCErrorResponse,deliver,cancelActiveRPCs,noteRetryAttempt,releaseSlot.session_test.go—fakeStream/fakeDescplus tests forhandleOpenSession,handleVRPC*,handleGoAway,handleErrorResponse,Invoke,ForceClose,Close,heartBeatLoop, PeerInfo extraction,AfeID,Start.Part 3c of 3 in the Session core sub-split. Stacks on:
metrics.TransportTypeNameexport.Until those merge, the diff shown here includes their file changes. The "own" scope of this PR is 3 files under
bigtable/internal/transport/.Review feedback folded in
All 8 findings from @gemini-code-assist on the previous revision (SHA
64e5d43c54) are addressed in this rewrite:onStartimmediately aftertransitionTo(StateStarting)so the "onStartprecedesonClose" invariant holds even whenSendfails andForceClosefiresonClose. (findings onsession_lifecycle.go:46,session_lifecycle.go:59)handleErrorResponse,handleSessionParameters,handleVRPCResponse,handleVRPCErrorResponse, and theres.respbranch inawaitInvokeResult; drop instead of panicking in thereadLoopgoroutine. (findings onsession_lifecycle.go:294,session_lifecycle.go:312,session_vrpc.go:227,session_vrpc.go:269,session_vrpc.go:297)Sendfailures inInvokeasStateUncommitted(wasStateTransportFailure) so the retry interceptor can retry non-idempotent ops when the frame never reached the server — Java parity with java-bigtable. (finding onsession_vrpc.go:130)Java parity notes
SessionState):New/Starting/Ready/Closing/WaitServerClose/Closed.Senderrors classify asStateUncommitted, matching java-bigtable.OK-gated tracker updates for transport EWMA matchSessionList.java:181-187.Follow-ups (in the queued order)
RetryingVRpc+ retryable classifier that consumesAttemptOutcome.session_list.go+ AFE picker (usesSessionHandle).session_snapshot.go.ConfigurationManager(rebase of feat(bigtable): add ClientConfigurationManager #19986).SessionClient/SessionTablefactory.debugviewhandler + per-view PRs (sessionz/afez/flightz/loadz/…).Test plan
go build ./bigtable/...go test ./bigtable/internal/transport/ -count=1 -short— passes (4.5s)gofmt -l bigtable/internal/transport/— clean