Skip to content

Commit d055a8a

Browse files
authored
feat(bigtable): add NoOpChannelPrimer for session channel pools (#20208)
## Summary Adds a `NoOpChannelPrimer` implementation of `ChannelPrimer` that explicitly disables per-connection priming. Session channel pools warm their channels via the OpenSession handshake on each newly-opened stream, so a PingAndWarm at dial time is redundant. The `ChannelPrimer` contract already allows nil-as-\"no priming\" (see the factory's nil-primer branch), but nil is a placeholder-shaped sentinel — easy to misread as \"primer not wired up yet\" rather than a deliberate architectural choice. `NoOpChannelPrimer` makes the intent explicit at the construction site and gives the follow-up SessionClient refactor a named type to plumb. ## Changes - **`channel_primer.go`** — new `NoOpChannelPrimer struct{}` + `Prime` method returning nil; interface docstring extended to name it alongside `pingAndWarmChannelPrimer`. - **`channel_primer_test.go`** — three new tests: - `TestNoOpChannelPrimer_Prime` — returns nil and doesn't dereference the connection. - `TestNoOpChannelPrimer_ImplementsChannelPrimer` — compile-time guard on interface satisfaction. - `TestConnectionFactory_NoOpPrimerSkipsPriming` — composes into the existing `connectionFactory` the same way nil does; no PingAndWarm on the wire. ## Test plan - [x] \`go test ./bigtable/internal/transport/ -run 'ChannelPrimer|Primer|Prime' -count=1 -short\` → 4/4 relevant + PingAndWarm suite all pass locally. - [x] \`go build ./bigtable/internal/transport/\` clean. - [x] \`go vet ./bigtable/internal/transport/\` clean. - [x] \`golint\` clean. - [x] CI green (post-rebase + consolidation).
1 parent 2fcfe5f commit d055a8a

2 files changed

Lines changed: 60 additions & 16 deletions

File tree

‎bigtable/internal/transport/channel_primer.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,13 +33,29 @@ import (
3333
// - pingAndWarmChannelPrimer issues a PingAndWarm against the configured
3434
// instance / app profile with the supplied feature-flag metadata
3535
// (today's only behavior, used by the classic channel pool factory).
36+
// - NoOpChannelPrimer skips priming entirely. Prefer this over nil at
37+
// construction sites where "no priming" is a deliberate architectural
38+
// choice (e.g. session channel pools that warm via OpenSession on
39+
// each stream) rather than a placeholder waiting for a real primer.
3640
type ChannelPrimer interface {
3741
// Prime warms conn so the next request served by it does not pay the
3842
// first-RPC connection-setup cost. The factory wraps Prime in a retry
3943
// loop, so transient errors should propagate as-is.
4044
Prime(ctx context.Context, conn *BigtableConn) error
4145
}
4246

47+
// NoOpChannelPrimer explicitly disables per-connection priming. Session
48+
// channel pools warm their channels via the OpenSession handshake on
49+
// each newly-opened stream, so a PingAndWarm at dial time is redundant.
50+
// Passing NoOpChannelPrimer{} makes the "no priming" choice explicit at
51+
// the construction site instead of relying on nil-as-sentinel.
52+
type NoOpChannelPrimer struct{}
53+
54+
// Prime is a no-op — the channel is returned unprimed.
55+
func (NoOpChannelPrimer) Prime(_ context.Context, _ *BigtableConn) error {
56+
return nil
57+
}
58+
4359
// pingAndWarmChannelPrimer primes a channel by issuing a PingAndWarm RPC
4460
// against the configured instance + app profile, carrying the supplied
4561
// feature-flag metadata. Stateless aside from the configured identifiers,

‎bigtable/internal/transport/channel_primer_test.go‎

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -79,25 +79,53 @@ func TestPingAndWarmChannelPrimer_Prime_NotFoundIsBenign(t *testing.T) {
7979
}
8080
}
8181

82-
// TestConnectionFactory_NilPrimerSkipsPriming verifies the contract that a
83-
// nil ChannelPrimer turns priming off: newEntry dials the channel and
84-
// returns it without issuing PingAndWarm.
85-
func TestConnectionFactory_NilPrimerSkipsPriming(t *testing.T) {
86-
fake := &fakeService{}
87-
addr := setupTestServer(t, fake)
88-
89-
factory := &connectionFactory{
90-
dial: func() (*BigtableConn, error) { return dialBigtableserver(addr) },
91-
primer: nil,
82+
// TestNoOpChannelPrimer_Prime verifies the primer returns nil without
83+
// dialing or touching the connection — a nil BigtableConn is safe to
84+
// pass because Prime never dereferences it.
85+
func TestNoOpChannelPrimer_Prime(t *testing.T) {
86+
var p NoOpChannelPrimer
87+
if err := p.Prime(context.Background(), nil); err != nil {
88+
t.Errorf("Prime returned err = %v, want nil", err)
9289
}
90+
}
9391

94-
entry, err := factory.newEntry(context.Background())
95-
if err != nil {
96-
t.Fatalf("newEntry returned error: %v", err)
92+
// TestNoOpChannelPrimer_ImplementsChannelPrimer is a compile-time guard:
93+
// NoOpChannelPrimer must satisfy the ChannelPrimer interface so
94+
// construction sites can pass it wherever a ChannelPrimer is expected.
95+
func TestNoOpChannelPrimer_ImplementsChannelPrimer(t *testing.T) {
96+
var _ ChannelPrimer = NoOpChannelPrimer{}
97+
}
98+
99+
// TestConnectionFactory_NoPrimingVariants verifies both no-prime primer
100+
// shapes turn PingAndWarm off: an untyped nil primer AND the explicit
101+
// NoOpChannelPrimer sentinel. newEntry dials the channel and returns
102+
// it without issuing PingAndWarm in either case.
103+
func TestConnectionFactory_NoPrimingVariants(t *testing.T) {
104+
cases := []struct {
105+
name string
106+
primer ChannelPrimer
107+
}{
108+
{"nil-primer", nil},
109+
{"no-op-primer", NoOpChannelPrimer{}},
97110
}
98-
t.Cleanup(func() { entry.conn.Close() })
111+
for _, tc := range cases {
112+
t.Run(tc.name, func(t *testing.T) {
113+
fake := &fakeService{}
114+
addr := setupTestServer(t, fake)
115+
factory := &connectionFactory{
116+
dial: func() (*BigtableConn, error) { return dialBigtableserver(addr) },
117+
primer: tc.primer,
118+
}
119+
120+
entry, err := factory.newEntry(context.Background())
121+
if err != nil {
122+
t.Fatalf("newEntry returned error: %v", err)
123+
}
124+
t.Cleanup(func() { entry.conn.Close() })
99125

100-
if got := fake.getPingCount(); got != 0 {
101-
t.Errorf("PingAndWarm call count with nil primer = %d, want 0", got)
126+
if got := fake.getPingCount(); got != 0 {
127+
t.Errorf("PingAndWarm call count = %d, want 0", got)
128+
}
129+
})
102130
}
103131
}

0 commit comments

Comments
 (0)