distribution: add split job management RPCs#1092
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 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 |
|
Implemented split job management RPCs and coordinator-backed lifecycle transitions. @codex review |
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 (b93f5b9):
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 new gRPC endpoints to DistributionServer for managing split migrations and split jobs, alongside a new split_job_lifecycle.go file to handle job state transitions and corresponding unit tests. The review feedback recommends exporting the splitJobsEquivalent helper function from the distribution package as SplitJobsEquivalent and reusing it within the adapter package to eliminate the duplicated splitJobsEqualByEncoding helper.
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 err != nil { | ||
| return splitJobCatalogStatusError(err) | ||
| } | ||
| if splitJobsEqualByEncoding(expected, next) { |
There was a problem hiding this comment.
Use the exported distribution.SplitJobsEquivalent helper instead of the duplicated local splitJobsEqualByEncoding function to improve maintainability.
| if splitJobsEqualByEncoding(expected, next) { | |
| if distribution.SplitJobsEquivalent(expected, next) { |
References
- Centralize common complex logic, such as retry/backoff mechanisms, into shared helper functions to avoid code duplication and improve maintainability.
| func splitJobsEqualByEncoding(left, right distribution.SplitJob) bool { | ||
| leftRaw, leftErr := distribution.EncodeSplitJob(left) | ||
| rightRaw, rightErr := distribution.EncodeSplitJob(right) | ||
| return leftErr == nil && rightErr == nil && bytes.Equal(leftRaw, rightRaw) | ||
| } |
There was a problem hiding this comment.
This helper function can be removed entirely since we can now use the exported distribution.SplitJobsEquivalent function.
References
- Centralize common complex logic, such as retry/backoff mechanisms, into shared helper functions to avoid code duplication and improve maintainability.
| if err != nil { | ||
| return SplitJob{}, err | ||
| } | ||
| if splitJobsEquivalent(expected, next) { |
There was a problem hiding this comment.
Update this call to use the exported SplitJobsEquivalent function.
| if splitJobsEquivalent(expected, next) { | |
| if SplitJobsEquivalent(expected, next) { |
| return false | ||
| } | ||
|
|
||
| func splitJobsEquivalent(left, right SplitJob) bool { |
There was a problem hiding this comment.
The helper function splitJobsEquivalent is identical in behavior to splitJobsEqualByEncoding in the adapter package. To avoid code duplication and improve maintainability, we should export splitJobsEquivalent as SplitJobsEquivalent so that it can be reused directly by the adapter package.
| func splitJobsEquivalent(left, right SplitJob) bool { | |
| // SplitJobsEquivalent returns true if the two SplitJobs have the same encoded representation. | |
| func SplitJobsEquivalent(left, right SplitJob) bool { |
References
- Centralize common complex logic, such as retry/backoff mechanisms, into shared helper functions to avoid code duplication and improve 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: b93f5b9d47
ℹ️ 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".
| resp := &pb.ListSplitJobsResponse{} | ||
| for _, job := range jobs { | ||
| if !splitJobPassesListFilter(job, req.GetSinceTerminalAtMs(), phaseFilter) { | ||
| continue | ||
| } | ||
| resp.Jobs = append(resp.Jobs, distribution.SplitJobToProto(job)) | ||
| } |
There was a problem hiding this comment.
When there are more than the designed default page of split jobs/history records, this implementation appends every matching job and never consumes page_cursor or sets next_page_cursor. The design for ListSplitJobs explicitly calls for live+history with a default cap of 200 newest and cursor-based paging (docs/design/2026_06_11_proposed_hotspot_split_milestone2_migration.md), so an operator with a large history gets an unbounded response and has no way to page through results.
Useful? React with 👍 / 👎.
| case errors.Is(err, kv.ErrLeaderNotFound): | ||
| return grpcStatusError(codes.FailedPrecondition, errDistributionNotLeader.Error()) |
There was a problem hiding this comment.
If this node loses catalog leadership after verifyCatalogLeader but before/during coordinator.Dispatch, kv.Coordinate.Dispatch can still surface transient leader errors such as raft ErrNotLeader, leadership lost, or forwarded not leader strings after its retry budget (kv/coordinator.go:isTransientLeaderError). This switch only recognizes kv.ErrLeaderNotFound, so those split-job retry/abandon calls are reported as Internal instead of the not-leader/failed-precondition response clients can retry against.
Useful? React with 👍 / 👎.
|
@codex review Updated in 58c2128:
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 (58c2128):
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: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Current head 58c2128 has green CI and no current-head inline blockers found in the latest check. Requesting current-head review.\n\n@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
## Summary - keep StartSplitMigration fail-closed until an explicit migration capability gate succeeds - create planned SplitJob records through the catalog coordinator when the gate is open - reject reserved migration/control ranges and enforce the one-live-job limit before writing migration state ## Tests - go test ./adapter -run 'TestDistributionServer(StartSplitMigration|SplitJobRPCs)' -count=1 - go test ./distribution -count=1 - golangci-lint --config=.golangci.yaml run ./adapter ./distribution --timeout=5m Note: a broader go test ./adapter ./distribution -count=1 run previously timed out in adapter after 10m during wider raft/gRPC tests; the targeted DistributionServer tests and distribution package pass.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 (aff2a9f):
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 Latest head has green CI and no current-head top-level inline findings from the review-comments API. Requesting a fresh review for the current head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aff2a9f037
ℹ️ 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".
…eadiness' into design/hotspot-split-m2-job-rpc # Conflicts: # adapter/distribution_server_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 (9994ec7):
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. |
|
Updated in 9994ec7. Changes:
Validation:
|
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Tests