perf(spanner): create admin stubs lazily instead of at construction#6
Open
fornwall wants to merge 2 commits into
Open
perf(spanner): create admin stubs lazily instead of at construction#6fornwall wants to merge 2 commits into
fornwall wants to merge 2 commits into
Conversation
GapicSpannerRpc eagerly created GrpcInstanceAdminStub and GrpcDatabaseAdminStub in its constructor. With the instantiating channel provider each admin stub builds its own transport channel (grpc-gcp pool, default 8), interceptor chain and watchdog - so a data-only client stood up three full channel stacks at startup even though the vast majority of applications never call the instance/database admin APIs (provisioning, DDL and backups are typically done out-of-band). Wrap both admin stubs in a small LazyAdminStub holder that builds the underlying gRPC stub on first use (double-checked locking). The (relatively cheap) stub settings are still built eagerly, so getInstanceAdminStubSettings()/getDatabaseAdminStubSettings() and the emulator auto-config path are unchanged. shutdown()/shutdownNow() only close the admin stubs if they were actually created. The auto-throttle GetOperation retry wiring for the database admin stub is moved verbatim into a createDatabaseAdminStub() helper invoked lazily. Effect: a data-only client now builds one channel stack at startup instead of three. The two channel/socket-counting tests are updated to assert the reduced footprint (2 grpc-gcp channels / 16 sockets instead of 6 / 48), with comments explaining the laziness. The existing admin Gax/ClientImpl tests (which drive admin RPCs through GapicSpannerRpc) continue to pass, exercising lazy creation on first use. Note: a bad admin stub configuration now surfaces on the first admin call rather than at Spanner construction; data-only clients no longer hit it at all. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ci2KFsNgWdHCno4dkXdUU
Address review findings on the lazy admin stub change: 1. Close the shutdown/get race: shutdown()/shutdownNow() now call LazyAdminStub.closeAndGet(), which atomically marks the holder closed and returns the stub if one was created. A get() racing shutdown either completes creation before closeAndGet() (so the stub is still closed by shutdown) or observes the closed flag and fails with FAILED_PRECONDITION instead of leaking a freshly created channel stack. 2. Catch Exception instead of only IOException in the lazy factory, preserving the constructor's previous asSpannerException wrapping for runtime exceptions from user-supplied providers. 3. Drop the dead null guard on the lazy holders in shutdownNow(), which contradicted the unconditional spannerStub.close() on the next line. 4. Rename testDirectPathFallbackCreatesExactlyFourPhysicalSockets to ...SixteenPhysicalSockets to match what it asserts. Adds tests pinning the new post-shutdown behavior and the shutdown path for a lazily created admin stub. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B1XiAgYMV1Y2SUx5wdxjfF
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.
Issue (review P2-7)
GapicSpannerRpcbuildsGrpcInstanceAdminStubandGrpcDatabaseAdminStubin its constructor (spi/v1/GapicSpannerRpc.java~505–563). With the instantiating channel provider, each admin stub builds its own transport channel (grpc-gcp pool, default 8 channels), interceptor chain, and watchdog. So a data-only client stands up three full channel stacks at startup — data, instance-admin, database-admin — even though the vast majority of applications never call the admin APIs.What the admin APIs are
These are provisioning/operational surfaces almost always driven out-of-band (Terraform,
gcloud, CI), not from the request-serving app process.SpannerImplalready exposes them behind getters, so making the stubs lazy is API-compatible.Fix
Wrap both admin stubs in a small
LazyAdminStub<T extends BackgroundResource>holder that builds the underlying gRPC stub on first use (double-checked locking,volatilefield):getInstanceAdminStubSettings()/getDatabaseAdminStubSettings()and the emulator auto-config path are unchanged.shutdown()/shutdownNow()callcloseAndGet(), which atomically marks the holder closed and returns the stub only if it was actually created, so a data-only client that never touched admin has nothing extra to tear down. Aget()racing shutdown either finishes creating the stub beforecloseAndGet()observes it (so shutdown still closes it), or sees the closed flag and fails withFAILED_PRECONDITION— no window in which a freshly created channel stack can leak.shutdown()now fails cleanly withFAILED_PRECONDITIONinstead of creating (and leaking) a fresh stub.GetOperationRESOURCE_EXHAUSTED retry wiring for the database admin stub is moved verbatim into a staticcreateDatabaseAdminStub()helper, invoked from the lazy factory.asSpannerExceptionconversion the constructor used.Effect
A data-only client now builds one channel stack at construction instead of three. Verified via the two channelz/socket-counting tests, updated to assert the reduced footprint:
(3 channel-building stubs → 1; the partitioned-DML stub shares the data stub's
ClientContext, so it adds none.)Tests
GapicSpannerRpcTest.testDirectPathFallbackCreatesOneGrpcGcpLayerPerPathandGapicSpannerRpcConnectionTest.testDirectPathFallbackCreatesExactlySixteenPhysicalSockets(renamed from the stale...ExactlyFourPhysicalSockets) updated to the new counts, with comments explaining that admin stubs are now lazy.GapicSpannerRpcTest.testAdminCallAfterShutdownFailsInsteadOfCreatingStubpins the post-shutdown behavior (FAILED_PRECONDITION, no stub creation) for both admin surfaces.GapicSpannerRpcTest.testShutdownClosesLazilyCreatedAdminStubexercises the shutdown path when an admin stub was lazily created.Gax/ClientImpltests (DatabaseAdminGaxTest,InstanceAdminGaxTest,DatabaseAdminClientImplTest,InstanceAdminClientImplTest) drive admin RPCs throughGapicSpannerRpcand continue to pass — exercising lazy creation on first use.Behavioral note
A bad admin stub configuration now surfaces on the first admin call rather than at
Spannerconstruction; data-only clients no longer hit it at all. Calling an admin method aftershutdown()fails withFAILED_PRECONDITION("Cannot use an admin stub after the client has been closed") instead of silently creating a new stub.Related (P3, not addressed here)
The review's P3 note — that all three stub-settings objects are also built eagerly in
SpannerOptions.newBuilder()and rebuilt in the constructor — is a separate classloading-cost cleanup and is left as a follow-up.🤖 Generated with Claude Code
https://claude.ai/code/session_016Ci2KFsNgWdHCno4dkXdUU
https://claude.ai/code/session_01B1XiAgYMV1Y2SUx5wdxjfF