Repository navigation
refactor(components): route SpannerStore transactions via Storage and drop SpannerClient - #1403
Merged
andrzejfudro merged 3 commits intoOct 29, 2026
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the component storage layer by abstracting Spanner-specific mutations out of the generic SpannerStore and into the repository layer, introducing new transaction interfaces and post-commit callbacks. The review feedback highlights several safety issues in the newly added Spanner mutations, including a potential nil pointer dereference on rec.ObservedGeneration, a potential marshaling failure with nil access payloads in UpdateComponentState, and missing map existence checks when retrieving dependency IDs in InsertComponent and UpdateComponentIntent.
andrzejfudro
force-pushed
the
refactor/cms-storage-mutations-and-tx
branch
from
October 23, 2026 13:56
ea460df to
8e21889
Compare
andrzejfudro
force-pushed
the
refactor/cms-storage-mutations-and-tx
branch
from
October 26, 2026 10:02
8e21889 to
97b4309
Compare
andrzejfudro
force-pushed
the
refactor/cms-storage-mutations-and-tx
branch
2 times, most recently
from
October 28, 2026 12:10
4336cbb to
488ca69
Compare
Base automatically changed from
refactor/cms-storage-snapshots-and-fence
to
main
October 28, 2026 13:14
andrzejfudro
force-pushed
the
refactor/cms-storage-mutations-and-tx
branch
2 times, most recently
from
October 28, 2026 15:32
54b5e04 to
8c22527
Compare
andrzejfudro
changed the base branch from
main
to
refactor/cms-storage-component-mutations
October 28, 2026 15:47
andrzejfudro
force-pushed
the
refactor/cms-storage-mutations-and-tx
branch
2 times, most recently
from
October 29, 2026 11:33
ae58678 to
0c35d3d
Compare
… drop SpannerClient Add RunInTransactionWithCommitTime to storage.TransactionRunner and WithReadOnlySnapshot to storage.ComponentRepository so SpannerStore and ownership fencing run all read-only snapshots and read-write transactions through storage.Storage without post-commit read-backs. Remove the *spanner.Client parameter from NewSpannerStore, drop SpannerClient from factory.Storage and ProdServer, and wire initCMS to construct SpannerStore whenever Storage is initialized. TAG=agy CONV=5d43b8db-2f9c-4908-a311-376a2b7dfdf0
andrzejfudro
force-pushed
the
refactor/cms-storage-mutations-and-tx
branch
from
October 29, 2026 11:50
0c35d3d to
a7090a4
Compare
andrzejfudro
marked this pull request as ready for review
October 29, 2026 11:52
VLukiantsev
reviewed
Oct 29, 2026
VLukiantsev
reviewed
Oct 29, 2026
VLukiantsev
reviewed
Oct 29, 2026
VLukiantsev
reviewed
Oct 29, 2026
…e contract Document RunInTransactionWithCommitTime, ComponentRepository.WithReadOnlySnapshot, and MockStorage.CommitTime in internal/storage/README.md, clarify the storage.Storage requirements on NewSpannerStore, and align the PostgreSQL RunInTransactionWithCommitTime stub comment with #1416. TAG=agy CONV=5d43b8db-2f9c-4908-a311-376a2b7dfdf0
andrzejfudro
enabled stacked auto-merge
October 29, 2026 12:40
VLukiantsev
previously approved these changes
Oct 29, 2026
Collaborator
There was a problem hiding this comment.
LGTM, approving on the condition that the CMS store-selection thread on cmd/experiment_state_server/main.go is addressed with one of the proposed low-effort options before merge (see discussion_r4412578759). Thanks for the quick doc fixes.
…ndSpanner Keep MemoryStore fallback for non-Spanner backends in initCMS until PostgreSQL component writes land (#1418), and add the Postgres fallback case to TestInitCMS_StoreSelection. Comment link: #1403 (comment) TAG=agy CONV=5d43b8db-2f9c-4908-a311-376a2b7dfdf0
There was a problem hiding this comment.
🤖 Incremental PR Approval
PR author is a code owner and changes since last approval (24 lines, excluding 43 test file lines) are within the 50-line grace delta.
Preserving prior approval by @VLukiantsev on commit afb984f.
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.
Summary
Route every
SpannerStoreread-only snapshot and read-write transaction throughstorage.Storage(including commit-timestamp reporting viaRunInTransactionWithCommitTime) and drop*spanner.Clientexposure fromNewSpannerStore,factory.Storage, andProdServer.Key Changes
internal/storage): AddRunInTransactionWithCommitTimetoTransactionRunner(returning the Spanner commit timestamp on Spanner,codes.Unimplementedon PostgreSQL, andMockStorage.CommitTimein tests) andWithReadOnlySnapshottoComponentRepository.SpannerStore(pkg/components/store/spanner): SwitchSpannerStoreand ownership fencing ontostorage.Storage(RunInTransaction,RunInTransactionWithCommitTime,WithReadOnlySnapshot) and drop the*spanner.Clientconstructor parameter without post-commit read-backs.internal/storage/factory,cmd/experiment_state_server): RemoveSpannerClientfromfactory.StorageandProdServer, exposeProdServer.StorageBackend(), and gateNewSpannerStore(st)ininitCMSonst != nil && backend == dbconfig.BackendSpannerso non-Spanner backends keep theMemoryStorefallback until PostgreSQL component writes land.Assumptions
RunInTransactionWithCommitTimereturnscodes.Unimplementedin this PR — PostgreSQL commit-timestamp support lands in the follow-up PostgreSQL serializable-transaction PR (feat(storage): add RunInSerializableTransaction for component catalog writes #1416) — callingRunInTransactionWithCommitTimeagainst PostgreSQL before that PR lands returnsUnimplemented.initCMStemporarily gatesSpannerStoreselection onbackend == dbconfig.BackendSpanner(falling back toMemoryStoreotherwise) — PostgreSQL component reads and writes are deferred to follow-up PRs (feat(storage): implement PostgreSQL component repository reads #1417–feat(storage): implement PostgreSQL component writes and run the dbstore suite on PostgreSQL #1418) — non-Spanner deployments withcomponentsDeployment.enabledcontinue using non-durableMemoryStoreuntil feat(storage): implement PostgreSQL component writes and run the dbstore suite on PostgreSQL #1418 removes the gate.health_servicestill builds its Spanner client andspannerstorage.NewStoragedirectly — movinglazy_spanner_client.goontodbfactory.NewStorageis a separate follow-up PR —health_serviceremains Spanner-bound until that follow-up lands.Verification
bazel test //internal/storage/... //pkg/components/... //cmd/experiment_state_server/... //cmd/health_service/...(Passed)./.githooks/pre-commit(Passed)TAG=agy
CONV=5d43b8db-2f9c-4908-a311-376a2b7dfdf0