migration: complete target promotion catalog state#1090
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 (351477d):
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 |
|
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 introduces the target promotion completion logic for split job migrations, including the state transition function CompleteTargetPromotionState and the transactional catalog store method CompleteSplitJobTargetPromotion. Feedback suggests strengthening defensive checks by validating the route's parent ID and start key during route clearing, and removing a redundant history lookup that could impact performance.
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.
| if !route.StagedVisibilityActive || route.GroupID != job.TargetGroupID { | ||
| return nil, errors.WithStack(ErrMigrationInvalidRoute) | ||
| } |
There was a problem hiding this comment.
To enforce defensive programming and ensure catalog consistency, we should also validate that the target route's ParentRouteID and Start key match the split job's SourceRouteID and SplitKey before clearing the staged visibility fields. This prevents accidentally promoting an inconsistent or incorrect route descriptor.
| if !route.StagedVisibilityActive || route.GroupID != job.TargetGroupID { | |
| return nil, errors.WithStack(ErrMigrationInvalidRoute) | |
| } | |
| if !route.StagedVisibilityActive || route.GroupID != job.TargetGroupID || route.ParentRouteID != job.SourceRouteID || !bytes.Equal(route.Start, job.SplitKey) { | |
| return nil, errors.WithStack(ErrMigrationInvalidRoute) | |
| } |
| if err := s.expectLiveSplitJobAt(ctx, expected.JobID, expectedRaw, readTS); err != nil { | ||
| return 0, 0, nil, err | ||
| } | ||
| if _, found, err := s.historySplitJobAt(ctx, expected.JobID, readTS); err != nil { | ||
| return 0, 0, nil, err | ||
| } else if found { | ||
| return 0, 0, nil, errors.WithStack(ErrCatalogSplitJobConflict) | ||
| } |
There was a problem hiding this comment.
The check s.historySplitJobAt is redundant and inefficient here. Since s.expectLiveSplitJobAt has already succeeded, the split job is guaranteed to be in the live state. In a consistent MVCC catalog, a split job cannot exist in both the live state and the history state simultaneously. Scanning the entire history of split jobs (which can grow very large in production) is expensive and unnecessary. We can safely remove this check.
| if err := s.expectLiveSplitJobAt(ctx, expected.JobID, expectedRaw, readTS); err != nil { | |
| return 0, 0, nil, err | |
| } | |
| if _, found, err := s.historySplitJobAt(ctx, expected.JobID, readTS); err != nil { | |
| return 0, 0, nil, err | |
| } else if found { | |
| return 0, 0, nil, errors.WithStack(ErrCatalogSplitJobConflict) | |
| } | |
| if err := s.expectLiveSplitJobAt(ctx, expected.JobID, expectedRaw, readTS); err != nil { | |
| return 0, 0, nil, err | |
| } |
References
- Remove dead or unreachable code paths, such as asynchronous handlers for tasks that are always performed synchronously, to improve code clarity and maintainability.
![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: 351477d4e1
ℹ️ 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".
| out.Changed = true | ||
| out.ClearedRouteIDs = cleared | ||
| out.Job.TargetPromotionDone = true | ||
| out.Job.PromotionCompletedTS = completionTS |
There was a problem hiding this comment.
In the promotion-complete path, this stores the caller-supplied completionTS in promotion_completed_ts, but the design contract for that field is the HLC commit timestamp of the default-group CAS that clears the descriptor (see docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md, promotion_completed_ts). In normal calls the target-local promotion timestamp/max promoted timestamp can differ from the catalog MVCC commit timestamp chosen later in applyPromotionCompleteMutations, so target cleared-descriptor ACKs can prove or wait on the wrong timestamp; compute the catalog commit timestamp before encoding the job and persist that value instead.
Useful? React with 👍 / 👎.
351477d to
c57e42a
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 (c57e42a):
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. |
c57e42a to
2edefb8
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 (2edefb8):
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48a4da82d3
ℹ️ 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".
| state.MigrationJobID = binary.BigEndian.Uint64(rest[:migrationUint64Bytes]) | ||
| rest = rest[migrationUint64Bytes:] | ||
| state.MinWriteTSExclusive = binary.BigEndian.Uint64(rest[:migrationUint64Bytes]) | ||
| return state, rest[migrationUint64Bytes:], true |
There was a problem hiding this comment.
When a node loads or restores a version-1 target readiness record, rest has already been advanced past the four fixed uint64 fields, so slicing rest[migrationUint64Bytes:] skips the first 8 bytes of the encoded route range. That makes valid V1 records fail decodeTargetReadinessRange, so a rolling upgrade or snapshot restore with retained V1 readiness metadata can reject the record as corrupt instead of preserving the migration fence state.
Useful? React with 👍 / 👎.
| if parent.State != distribution.RouteStateActive { | ||
| return distribution.RouteDescriptor{}, grpcStatusError(codes.FailedPrecondition, errDistributionSourceRouteNotActive.Error()) | ||
| } | ||
| splitKey := distribution.CloneBytes(req.GetSplitKey()) |
There was a problem hiding this comment.
When StartSplitMigration is used for filesystem chunk-domain splits, this keeps the raw RPC split key instead of normalizing it the way SplitRange does. A raw chunk key is rejected as outside the virtual route interval, and an in-domain key such as a chunk route plus a suffix can be persisted as the job split key, allowing the async migration path to bisect one file's chunk route and bypass the existing file-pinned hotspot guard; normalize once and use that value for validation and job creation.
Useful? React with 👍 / 👎.
|
|
||
| func migrationMutationsIntersect(muts []*pb.Mutation, routeStart []byte, routeEnd []byte) bool { | ||
| for _, mut := range muts { | ||
| if mut == nil || len(mut.Key) == 0 || isTxnInternalKey(mut.Key) { |
There was a problem hiding this comment.
When the source write tracker is armed and a supported DEL_PREFIX/flush-all mutation uses an empty key, this guard skips it before routePrefixRange can map it to the whole route keyspace. An in-flight flush with a commit timestamp below the migration snapshot can therefore be omitted from MinAdmittedTS, letting delta copy keep the snapshot floor and miss tombstones for keys that were already backfilled.
Useful? React with 👍 / 👎.
## Summary - promote the hotspot split M2 migration design from partial to implemented - record the completed production runner, current-voter barriers, route cutover, cleanup, DONE history, and deterministic Jepsen workload - update the parent hotspot design and scaling roadmaps to the implemented path and current status ## Evidence - PR #1096 merged the final M2 runner/lifecycle slice into this stacked base - latest #1096 head review reported no major issues and all GitHub checks passed - targeted Go migration, voter barrier, cleanup, promotion, and route tests passed - `lein test elastickv.split-workload-test` passed (2 tests, 5 assertions) - old partial filename references are absent ## Remaining scope - M3 owns automatic hotspot detection, target selection, and split scheduling - M4 owns the broader route-shuffle nemesis matrix and production-scale fault campaigns - reverse migration after CUTOVER and concurrent migration jobs remain future extensions
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 (4ed488c):
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 (ebf6878):
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 (f79bc00):
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. |
|
Addressed the latest-head review findings in f79bc00:
Verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f79bc00762
ℹ️ 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".
| } | ||
| err := s.updateSplitJobViaCoordinator(ctx, job.JobID, func(current distribution.SplitJob) (distribution.SplitJob, error) { | ||
| if current.Phase == job.Phase { | ||
| current.CapabilityRegressed = true |
There was a problem hiding this comment.
When the cluster capability gate starts failing after a split job has already armed migration side effects, this path only records CapabilityRegressed and pauses the runner; a repo-wide search shows the bit is not consulted by read/write routing or apply paths. In that downgraded-voter window, coordinators can keep serving the moving range while a regressed participant may ignore the write tracker/fences, so resuming the job later can compute an unsafe delta floor and miss writes. Please make this state fail closed for the moving route, or restrict it to phases before any migration guards/imports have been installed.
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 (4163665):
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. |
|
Addressed the latest-head capability regression fail-closed review in 4163665. Changes:
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 416366501b
ℹ️ 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 (d57699d):
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. |
|
Addressed the latest-head review items in d57699d. Changes:
Caller audit:
Verification:
Note: go test ./adapter ./kv ./store -count=1 hit the adapter package's default 10m timeout in broad integration coverage after the targeted and package compile checks had passed, so I did not treat that as evidence for this focused change. @codex review |
|
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: d57699d5a7
ℹ️ 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 err != nil || !metadataComplete { | ||
| return err |
There was a problem hiding this comment.
When any source voter still reports Ready=false for METADATA_CLEARED, syncSplitMigrationVoterBarrier returns metadataComplete=false with a nil error. This branch returns that nil error, so cleanupSplitJobSourceProofs treats the metadata cleanup as successful and advances the job past the retry point, releasing the retention pin and allowing the split to finish while a lagging source voter may still retain migration fence/tracker metadata. Return an explicit incomplete signal here instead of success.
Useful? React with 👍 / 👎.
|
@codex review Addressed the latest-head metadata cleanup and MVCC snapshot findings in dcb7744. Changes:
Semantic audit:
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 (dcb7744):
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: dcb7744f35
ℹ️ 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".
| Ready: time.Now().UnixMilli() >= req.GetReadDrainNotBeforeMs() && | ||
| (i.readTracker == nil || i.readTracker.Oldest() == 0), |
There was a problem hiding this comment.
When a split reaches source-read-drain cleanup, this probe only reports ready if the process-wide ActiveTimestampTracker is completely empty. In production the same tracker is wired into the Internal service and all frontend read paths, so any unrelated Redis/Dynamo/S3 read on this node keeps Oldest() nonzero even if it started after cutover or targets another range. On a busy node this can keep SOURCE_READ_DRAINED false indefinitely, preventing source cleanup/metadata cleanup and leaving the migration retention pin in place; the drain check needs to distinguish pre-cutover reads for the moving range (or at least reads older than the cutover/grace point) instead of requiring zero global reads.
Useful? React with 👍 / 👎.
Author: bootjp
Summary:
Tests: