migration: add range version RPC handlers#1088
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (64dc6f4):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for range migrations by implementing ExportRangeVersions and ImportRangeVersions on the internal gRPC server, alongside updating the distribution Route metadata to track migration states. Feedback focuses on adding a defensive check in ExportRangeVersions to prevent infinite loops when the cursor does not progress, and removing an unreachable nil guard on i.leader in verifyInternalLeader to comply with guidelines against dead defensive code, which also requires updating the corresponding unit tests to use a mock leader.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64dc6f4239
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (59a1672):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Updated in 59a1672:
Validation:
Note: a broader go test ./adapter ./distribution ./kv ./store -count=1 -timeout=240s timed out in the adapter integration suite; the targeted touched-path tests above passed. |
bootjp
force-pushed
the
design/hotspot-split-m2-cross-group
branch
from
59a1672 to
fbd7f56
Compare
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (fbd7f56):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review Rebased onto the latest design/hotspot-split-m2-fence-drain head in fbd7f56 after #1087 advanced. The migration import/export fixes remain intact and now include the parent broad DEL_PREFIX fence fix. Validation:
|
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Updated in 0fe341d:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (0fe341d):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (7d3b01c):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Updated in 7d3b01c:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (e7f69ef):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Updated in e7f69ef:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7f69efd9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func latestMVCCVersionAt(ctx context.Context, st store.MVCCStore, key []byte, ts uint64) (store.MVCCVersion, bool, error) { | ||
| result, err := st.ExportVersions(ctx, store.ExportVersionsOptions{ | ||
| StartKey: key, | ||
| EndKey: nextScanCursor(key), |
There was a problem hiding this comment.
When the backing store is Pebble, ExportVersions converts EndKey to encodeKey(EndKey, MaxUint64) (store/lsm_migration.go), and nextScanCursor(key) (key + 0x00) sorts before normal encoded versions of key because the timestamp suffix begins with ^commitTS. On a staged-visible route backed by Pebble, both the live and staged point exports therefore return no candidates for ordinary keys, so GetAt can incorrectly return ErrKeyNotFound even though the key exists. Use a true single-key upper bound/exact-key filter instead of the scan cursor here.
Useful? React with 👍 / 👎.
| ts uint64, | ||
| reverse bool, | ||
| ) ([]*store.KVPair, error) { | ||
| live, err := collectLatestLogicalVersions(ctx, g.Store, start, end, start, end, ts, liveLogicalVersionKey) |
There was a problem hiding this comment.
For staged-visible routes, this call starts an export loop that runs until result.Done, and the caller only applies limit after collecting both the full live range and the full staged prefix into maps. A user scan such as ScanAt(..., limit=100) over a migrated production range can therefore read and retain every version in the route before returning one page, which is a large latency/memory regression versus the previous bounded store scan. Please stop the merge once enough ordered candidates are available or otherwise page the merge by the requested limit.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (ca1a050):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review Updated in ca1a050:
Validation:
|
…ain' into design/hotspot-split-m2-cross-group # Conflicts: # distribution/engine.go # distribution/migrator.go # internal/s3keys/keys_test.go # kv/shard_store.go
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (727c1dd):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Merged latest design/hotspot-split-m2-fence-drain into this branch in 727c1dd and resolved conflicts without dropping either the read-fence route-version path or staged migration visibility. Conflict resolution notes:
Validation:
Caller audit:
Note: broad go test ./adapter ./store -count=1 -timeout=300s timed out in the adapter integration suite; store passed before adapter hit the package timeout, and the touched-path adapter tests above passed. |
…ain' into design/hotspot-split-m2-cross-group # Conflicts: # kv/fsm_migration_fence_test.go
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (db7a325):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Merged latest design/hotspot-split-m2-fence-drain into this branch in db7a325 after #1087 added the S3 bucket auxiliary write-fence fix. Conflict resolution:
Validation:
Caller audit:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db7a3253e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !ok { | ||
| return false | ||
| } | ||
| routeKey := s3keys.RouteKey(bucket, 0, "") |
There was a problem hiding this comment.
When exporting MigrationFamilyS3BucketMeta/MigrationFamilyS3BucketGeneration, this predicate checks only the single key RouteKey(bucket, 0, ""). If a split migrates a suffix inside the same bucket route prefix (for example a generation/object range after that sentinel), the bucket auxiliary row still needs to move because the bucket prefix intersects the route, but this returns false and the target can import objects without their bucket metadata/generation. Use the bucket-any-generation prefix intersection logic instead of one concrete generation-0 route key.
Useful? React with 👍 / 👎.
| return err | ||
| } | ||
| r.Ts = ts | ||
| if err := i.rejectWriteTimestampFloorMutations(r.Mutations, r.Ts); err != nil { |
There was a problem hiding this comment.
This floor check only runs when the leader assigns a missing raw Ts; forwarded raw requests that already have Ts skip the r.Ts != 0 case above and are committed without consulting the leader's current route floor. During migration, a follower with a stale catalog can forward a write timestamped at or below MinWriteTSExclusive, bypassing the invariant this helper is meant to enforce and allowing an old-timestamp write to land after cutover. Run the floor check for every raw request after Ts is known, not only for newly stamped ones.
Useful? React with 👍 / 👎.
| func (s *ShardStore) scanRouteAtLeaderRouteFilter( | ||
| ctx context.Context, | ||
| g *ShardGroup, | ||
| route distribution.Route, |
There was a problem hiding this comment.
The route-filtered leader path now receives the route, but it still pages from g.Store.ScanAt/ReverseScanAt directly and only uses the route later for lock materialization. When RouteBoundsPresent is used against a route with StagedVisibilityActive, staged-only versions are never considered in filteredKVs, so bounded raw-prefix scans can miss migrated data even though the non-filtered path calls scanRouteWithStagedVisibility. Route-filtered scans should take the same staged/live merge path before applying the route-key filter.
Useful? React with 👍 / 👎.
| continue | ||
| } | ||
| rkey := routeKey(elem.Key) | ||
| route, ok := c.engine.GetRoute(rkey) |
There was a problem hiding this comment.
This floor check only looks up the route for routeKey(elem.Key). For S3 bucket metadata/generation keys that is the raw !s3|bucket|... key, not the bucket's !s3route|... object-route prefix, so a bucket auxiliary write at or below MinWriteTSExclusive can bypass the floor even though the matching write-fence path checks s3BucketAuxiliaryRouteRange. Mirror the auxiliary range check in the point/mutation floor helpers so bucket metadata cannot be written with old timestamps during S3 route migration.
Useful? React with 👍 / 👎.
| return nil | ||
| } | ||
|
|
||
| func (i *Internal) rejectWriteTimestampFloorMutations(muts []*pb.Mutation, commitTS uint64) error { |
There was a problem hiding this comment.
This route-floor helper is only wired into stampRawTimestamps; when ForwardRequest.IsTxn is true, stampTxnTimestamps assigns the transaction commit timestamp in the txn meta and then forwards the batch without checking the leader's current MinWriteTSExclusive. A follower with a stale catalog can therefore forward a one-phase transaction for a route that the leader has already floor-fenced, and the FSM replay path allows old timestamps, so the write can commit below the cutover floor. Apply the same floor validation to txn mutations after the commit timestamp is known.
Useful? React with 👍 / 👎.
| return s.getGroupAtWithReadFence(ctx, groupID, key, ts, readRouteVersion) | ||
| } | ||
| g, ok := s.groupForKey(key) | ||
| route, g, ok := s.routeAndGroupForKey(key) |
There was a problem hiding this comment.
GetAt now decides whether to merge staged rows from routeAndGroupForKey(key), but S3 bucket metadata/generation keys do not normalize to the bucket's !s3route|... object-route prefix. During an S3 route migration where those auxiliary rows are imported under MigrationStagedDataKey, loadBucketMetaAt/nextBucketGenerationAt resolve the raw !s3|bucket|... route, skip getAtWithStagedVisibility, and can report a missing or stale bucket after cutover. Use the S3 auxiliary route-range lookup when selecting the staged-visibility route for these point reads.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (295cdf6):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Updated in 295cdf6:
Validation:
Caller audit:
Note: broad go test ./adapter ./kv -count=1 -timeout=300s timed out in the adapter integration suite; kv completed successfully and the touched-path adapter tests above passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 295cdf6bde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| func (s *ShardStore) routeAndGroupForKey(key []byte) (distribution.Route, *ShardGroup, bool) { | ||
| if route, ok := s.stagedVisibilityRouteForS3BucketAuxiliaryKey(key); ok { |
There was a problem hiding this comment.
When an S3 bucket route has staged visibility and the raw bucket-metadata keyspace is owned by a different group, this branch makes ShardStore.GetAt(BucketMetaKey/GenerationKey) read and merge from route.GroupID, but S3 bucket updates still enter through ShardedCoordinator.groupMutations → ShardRouter.ResolveGroup(routeKey(rawKey)), which does not use s3BucketAuxiliaryRouteRange. A bucket-fence write from PutObject/CreateBucket with a commit timestamp above the floor can therefore commit on the raw owner while subsequent reads are redirected here to the staged target and never see it; the write path needs the same auxiliary-route resolution before enabling this read redirect.
Useful? React with 👍 / 👎.
| if _, _, ok := distribution.MigrationStagedDataKeyParts(key); ok { | ||
| return nil, false | ||
| } | ||
| route, ok := c.engine.GetRoute(routeKey(key)) |
There was a problem hiding this comment.
For S3 bucket meta/generation keys, routeKey(key) remains the raw !s3|bucket|... key because the S3 extractor only normalizes object-scoped keys, so this helper returns no staged alias even while ShardStore reads merge staged bucket auxiliary rows. In a transaction that updates BucketMetaKey during staged visibility, the Raft request’s ReadKeys miss the !dist|migstage|... version, so Pebble OCC can fail to notice an imported staged version newer than startTS and allow a stale bucket metadata overwrite; this needs the same auxiliary-route lookup used by the point-read path.
Useful? React with 👍 / 👎.
| if route.MinWriteTSExclusive == 0 || commitTS > route.MinWriteTSExclusive { | ||
| return false | ||
| } | ||
| return kv.RouteKeyFilter(route.Start, route.End)(key) |
There was a problem hiding this comment.
This helper treats every forwarded raw mutation as a point key. When a follower forwards a pre-stamped DEL_PREFIX/FLUSHALL whose prefix intersects a route with commitTS <= MinWriteTSExclusive (for example, an empty prefix while a migrated route starts at m), this route-key predicate tests the prefix bytes themselves and returns false. The per-group FSM only checks write fences before applying prefix tombstones to its local Pebble store, so forwarded prefix deletes can still land below the migration floor unless DEL_PREFIX uses the same route-prefix intersection check as the coordinator path.
Useful? React with 👍 / 👎.
|
|
||
| func decodedS3BucketRouteIntersects(bucket string, routeStart, routeEnd []byte) bool { | ||
| bucketRouteStart := s3keys.RoutePrefixForBucketAnyGeneration(bucket) | ||
| return rangesIntersect(routeStart, routeEnd, bucketRouteStart, prefixScanEnd(bucketRouteStart)) |
There was a problem hiding this comment.
When exporting the S3 bucket meta/generation families with a route bound that covers the raw S3 namespace (for example ["!s3|", +∞) during an internal-prefix migration), this decoded-only predicate tests only the synthetic !s3route|bucket... interval. RouteKeyFilter/MigrationBracket.ContainsRoutedKey also accept the raw bucket key when it is directly in the route range, so this RPC path skips those bucket metadata/generation rows and the target can promote without the bucket-level state.
Useful? React with 👍 / 👎.
| if req == nil { | ||
| return nil, errors.WithStack(status.Error(codes.InvalidArgument, "import range versions request is nil")) | ||
| } | ||
| if i.migrationProposer == nil { |
There was a problem hiding this comment.
If an ImportRangeVersions caller omits job_id (and similarly bracket_id), this handler still proposes and acknowledges the batch. The imported versions are then staged under job 0, but staged visibility only activates for routes with a non-zero MigrationJobID, so the target can permanently ack data that no promoted route will read; validate the identifiers before proposing the Raft command.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (650c450):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
@codex review Updated in 650c450:
Caller audit:
Validation:
Note: go test ./adapter ./kv -count=1 -timeout=300s timed out in adapter on existing long-running tests TestRedis_StreamXReadLatencyIsConstant, Test_consistency_satisfy_write_after_read_sequence, and Test_grpc_transaction; kv completed successfully in that run. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 650c45072f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stagedKey := distribution.MigrationStagedDataKey(route.MigrationJobID, key) | ||
| staged, stagedOK, err := latestMVCCVersionAt(ctx, g.Store, stagedKey, ts) |
There was a problem hiding this comment.
When staged visibility is active and a non-empty DEL_PREFIX is applied after cutover, the apply path still deletes only the raw prefix, but imported rows that exist only under !dist|migstage|<job>|<rawKey> do not match that raw prefix. This merge then continues to read the staged alias with no newer live tombstone, so prefix deletes such as bucket/list cleanup can leave migrated-but-not-promoted keys visible; the delete path needs to also tombstone/delete the corresponding staged prefix (including the staged form of any exclude prefix) or materialize raw tombstones for staged-only keys.
Useful? React with 👍 / 👎.
| if !ok { | ||
| continue | ||
| } | ||
| if err := i.rejectWriteTimestampFloorMutations(r.Mutations, commitTS); err != nil { |
There was a problem hiding this comment.
For forwarded transactions on clusters where an unrelated low route has a migration write floor, this checks the synthetic txn metadata mutation along with user writes. txnMetaMutation uses the bare !txn|meta| key, which routeKey maps to the empty key, so any floor on the first route can reject a transaction whose actual writes are all on a different route with no floor; skip the metadata mutation here and apply the floor check only to the real write keys.
Useful? React with 👍 / 👎.
| start, end, ok := s3BucketAuxiliaryRouteRange(key) | ||
| if !ok { | ||
| return distribution.Route{}, false | ||
| } | ||
| for _, route := range s.engine.GetIntersectingRoutes(start, end) { |
There was a problem hiding this comment.
This adds the S3 bucket auxiliary staged route lookup only for point-key routing, but bucket listing paths scan the raw !s3|bucket|meta| prefix via ScanAt. Those scans still choose routes by intersecting the raw range, which does not overlap the staged owner’s !s3route|<bucket> range, so bucket metadata imported to the target group remains invisible to ListBuckets/AdminListBuckets until promotion even though point reads are merged.
Useful? React with 👍 / 👎.
| if route, ok := c.engine.GetRoute(rkey); ok && route.MinWriteTSExclusive != 0 && commitTS <= route.MinWriteTSExclusive { | ||
| return errors.Wrapf(ErrRouteWriteTimestampTooLow, "key %q routeKey %q commit_ts=%d floor=%d", key, rkey, commitTS, route.MinWriteTSExclusive) |
There was a problem hiding this comment.
For S3 bucket meta/generation keys, ownership is determined by s3BucketAuxiliaryRouteRange, but this raw routeKey floor check runs first. If an unrelated raw !s3|... range has a migration floor, bucket auxiliary writes for buckets whose !s3route|... owner is not fenced are rejected before the auxiliary-route check can decide, unlike the shard-store path which checks only the staged/bucket route for these keys.
Useful? React with 👍 / 👎.
| stagedStart, stagedEnd := stagedVisibilityScanBounds(route.MigrationJobID, start, end) | ||
| window := stagedVisibilityCandidateWindow(limit) | ||
| for { | ||
| liveKVs, err := scanVisibleCandidates(ctx, g.Store, start, end, window, ts, reverse) |
There was a problem hiding this comment.
When a staged-visibility scan's raw range includes !dist|migstage| (for example a full raw scan), the live candidate scan reads those shadow rows as ordinary keys before the separate staged decode pass. stagedVisibilityCandidateKeys will then materialize both the decoded raw key and the !dist|migstage|... key itself, leaking migration control rows to clients; filter staged-data keys out of liveKVs before merging.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (19a28e9):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Updated in 19a28e9:
Review candidate audit:
Validation:
|
…ain' into design/hotspot-split-m2-cross-group
|
@codex review Merged latest design/hotspot-split-m2-fence-drain into this branch in 1db42a8 after the #1087 retry/fence fixes landed. Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (1db42a8):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1db42a8715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if route, ok := s.stagedVisibilityRouteForS3BucketAuxiliaryKey(key); ok { | ||
| g, ok := s.groups[route.GroupID] | ||
| return route, g, ok |
There was a problem hiding this comment.
This point-key special case makes BucketMetaKey/BucketGenerationKey reads resolve to the staged owner, but prefix scans still go through routesForScan with the raw !s3|bucket|meta| bounds used by adapter/s3.go listBuckets and adapter/s3_admin.go AdminListBuckets. Those raw prefixes do not intersect the decoded !s3route|<bucket> route, so after cutover a bucket whose metadata only exists under MigrationStagedDataKey is missing from ListBuckets/admin scans until a live metadata write happens. Please add the same decoded S3 auxiliary route handling to scan route selection.
Useful? React with 👍 / 👎.
| if !ok { | ||
| return distribution.Route{}, false | ||
| } | ||
| route, ok := snap.RouteOf(routeKey(key)) |
There was a problem hiding this comment.
For S3 bucket metadata/generation primary keys, routeKey(key) remains the raw !s3|bucket|... key while the staged owner is found via the decoded !s3route|<bucket> range handled elsewhere by s3BucketAuxiliaryRouteRange. A one-phase retry whose prior attempt was imported as a staged version for such a primary misses this staged CommittedVersionAt probe and then applies the reused write set again at the fresh commit_ts, defeating the dedup path. Please resolve S3 auxiliary routes here before falling back to routeKey.
Useful? React with 👍 / 👎.
| if kvp == nil { | ||
| continue | ||
| } | ||
| seen[string(kvp.Key)] = bytes.Clone(kvp.Key) |
There was a problem hiding this comment.
When a staged-visibility scan range includes the reserved !dist|migstage| prefix (for example an unbounded/raw full scan), the live candidate scan can return the shadow rows themselves; adding those keys here makes visibleLogicalKVs emit both the internal staged key and the logical raw key. That leaks migration staging rows and duplicates results instead of keeping staged data visible only through the original key, so staged shadow keys need to be filtered out of the live-candidate path.
Useful? React with 👍 / 👎.
| if mut == nil || (len(mut.Key) == 0 && mut.GetOp() != pb.Op_DEL_PREFIX) { | ||
| continue |
There was a problem hiding this comment.
When Internal.Forward handles a transaction, each request includes the synthetic !txn|meta| mutation before the real write keys. This loop now applies route timestamp floors to that metadata key too; routeKey strips the prefix to an empty key, so any forwarded transaction with commit_ts below a floor on the first route is rejected even when all actual mutations belong to an unrelated route. Skip txn metadata here or check only the decoded write keys.
Useful? React with 👍 / 👎.
| EndKey: prefixScanEnd(key), | ||
| MaxCommitTSInclusive: ts, | ||
| MaxVersions: 1, | ||
| MaxScannedBytes: 0, |
There was a problem hiding this comment.
Leaving MaxScannedBytes at zero makes ExportVersions install the sparse-scan budget, so a single large/newer version above the requested read timestamp can return Done=false with no eligible Versions. latestMVCCVersionAt then treats that first partial page as not found instead of following NextCursor, causing staged-visibility point reads/scans at older timestamps to drop existing older versions. Loop until a version is found or the export reports done.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests