Repository navigation
refactor(components): migrate snapshots, fast-path read, and fence queries to storage - #1402
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the component storage layer by moving Spanner-specific queries and transaction operations—such as ownership fence management, snapshot retrieval, and component reassignment—from the high-level Spanner store into the dedicated component repository. It also updates the storage interfaces and adds corresponding unimplemented stubs to the Postgres repository. The review feedback highlights a critical reliability issue where the newly introduced ReadFence, WriteFence, and ReassignOwnedComponents methods directly access the transaction field r.tx without checking if it is nil, which can lead to nil pointer dereference panics when the repository is bound to a read-only transaction.
ec21b8e to
d4a2c17
Compare
705569b to
61ef7ca
Compare
61ef7ca to
75943aa
Compare
75943aa to
737d82e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the storage layer by moving component ownership fence and snapshot operations into the repository layer, specifically updating the ComponentRepository and ComponentTxRepository interfaces. These operations are implemented for the Spanner storage backend, while the Postgres storage backend is updated with unimplemented stubs. Additionally, SpannerStore is refactored to delegate these queries to the updated repository interfaces. I have no feedback to provide on these changes.
00ac183 to
4bf0130
Compare
…eries to storage Move historical snapshot reads (GetSnapshotByID, GetSnapshotByName), fast-path component+fence read (GetComponentWithFence), ownership gauge queries (ReadOwnershipGauges), and transactional ownership fence primitives (ReadFence, WriteFence, ReassignOwnedComponents) out of SpannerStore into internal/storage/spanner/component (with PostgreSQL Unimplemented stubs), deriving snapshot dependencies in SpannerStore outside the storage layer. Follow the Repository/TxRepository split from #1393 and guard ReadFence, WriteFence, and ReassignOwnedComponents against nil read-write transactions. TAG=agy CONV=5d43b8db-2f9c-4908-a311-376a2b7dfdf0
4bf0130 to
2b92d48
Compare
…torage - Drop non-transactional GetSnapshotByID and GetSnapshotByName from storage.ComponentRepository and Spanner Repository, keeping them only on storage.ComponentTxRepository with full contract docs. - Split WriteFence into RefreshFence(ctx, newName) and ReplaceFence(ctx, cluster), moving the rename check into declareClusterEpochTx. - Clarify ComponentTxRepository doc for read-only bindings and absent fence row locking. - Assert codes.NotFound and grpcerr.ErrFailedPrecondition in repository unit tests, and assert StaleOwners, ReassignOwnedComponents count, and ReadFence after RefreshFence. TAG=agy CONV=5d43b8db-2f9c-4908-a311-376a2b7dfdf0
There was a problem hiding this comment.
LGTM. Verified all review threads against 1872317: fixes applied cleanly and moved methods preserve their SQL, error, and transaction contracts.
Summary
GetComponentWithFence) and ownership gauge queries (ReadOwnershipGauges) fromSpannerStoreintointernal/storage/spanner/component.Repository(withcodes.Unimplementedstubs ininternal/storage/postgres/component.Repository).storage.ComponentTxRepository(internal/storage/spanner/component.TxRepository) withListActiveComponents, transactional snapshot reads (GetSnapshotByID,GetSnapshotByName), and ownership fence operations (ReadFence,RefreshFence,ReplaceFence,ReassignOwnedComponents), guarding read-write operations againstniltransactions withgrpcerr.ErrFailedPrecondition.withSnapshotDependencies) inSpannerStoresointernal/storageremains independent ofpkg/components/store/dependency.Testing
bazel test //internal/storage/spanner/component:component_test //internal/storage/postgres/component:component_test //pkg/components/store/spanner:spanner_testAssumptions
SpannerStoretemporarily retainsclient *spanner.Clientandrepo *component.Repository(marked with aTODO) to bindTxRepositoryinsideResolveComponentSnapshotand read-write transactions (DeclareClusterEpoch,SetComponentIntent,PrepareDeactivation,CompleteDeactivation,UpdateObservedGeneration,ExecuteDeletion) until refactor(components): route SpannerStore transactions via Storage and drop SpannerClient #1403 moves component mutations and transactions behindstorage.TransactionRunner.TAG=agy
CONV=5d43b8db-2f9c-4908-a311-376a2b7dfdf0