Fence Backfiller tasks by generation - #11311
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
chaptersix
changed the title
![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: 18eea9ba96
ℹ️ 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 taskGeneration == 0 { | ||
| // An old binary schedules generation-zero tasks and advances only Attempt. | ||
| valid = currentGeneration == 0 || attempt >= currentGeneration |
There was a problem hiding this comment.
During a rolling upgrade, a forward-dated backfill can still stall before this compatibility branch is reached: if an old binary receives the generation-bearing continuation, its old Validate ignores the generation and calls validateTaskHighWaterMark; because LastProcessedTime is in the future while the delayed task uses wall-clock time, it rejects and removes the continuation without scheduling the generation-zero successor assumed here. The mixed-version test only increments Attempt and therefore skips this real old-validator path; preserve a continuation across that handoff or explicitly gate mixed-version processing.
Useful? React with 👍 / 👎.
LastProcessedTime is a schedule-range cursor and cannot identify stale task deliveries. Stamp Backfiller tasks with a generation so superseded and redelivered tasks are rejected, while preserving generation-zero continuations during rolling upgrades.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
| int64 attempt = 8; | ||
|
|
||
| // Fence shared with the currently scheduled BackfillerTask. | ||
| int64 task_generation = 9; |
There was a problem hiding this comment.
Do you mind adding a bit more context as to the purpose of this field:
- When it gets incremented, what should increment it, what incrementing indicates
- The intent of the field - what it's present to guard against
- How is this different from attempt count?
Re naming: just a consistency check, do we use the term 'generation' to refer to a monotonic counter for this purpose? I don't have strong feelings, but I've heard version used like this, it's not clear to me if this is any clearer.
I guess version avoids the dual meanings that generation implies (as a kind of age-related cohort, versus the act of creating a new thing; to generate)? I don't feel very strongly about the naming thing though.
| message BackfillerTask {} | ||
| message BackfillerTask { | ||
| // Generation used to fence superseded tasks. | ||
| int64 generation = 1; |
| taskStamp := task.GetStamp() | ||
| currentStamp := backfiller.GetTaskStamp() | ||
| attempt := backfiller.GetAttempt() | ||
| valid := taskStamp == currentStamp && currentStamp > attempt |
There was a problem hiding this comment.
Sorry it took me so long to get my head around this. Nonblocking, we can fix / land this later if it's useful.
A simplification (not from me) that might be easier:
// A stamped task is outstanding while TaskStamp is ahead of Attempt. Attempt
// catches up to TaskStamp when a task executes without scheduling a successor
// (the Backfiller completed), and when an older binary executes a stamped task
// without advancing TaskStamp.
outstanding := currentStamp > attempt
var valid bool
if taskStamp == 0 {
// An old binary schedules zero-stamp tasks and advances only Attempt.
// Honour one only while no stamped task is outstanding to supersede it.
valid = !outstanding
} else {
valid = outstanding && taskStamp == currentStamp
}
|
I tested the case with multiple backfillers also, seems good, so I'm not overly worried about that. We can follow up with some additional testing later. |
3 participants
What changed?
Why?
LastProcessedTimetracks progress through the requested schedule range, while a task'sScheduledTimecontrols when that task runs. Comparing the two can either keep an already-processed historical task valid or reject a forward-dated task before it runs.How did you test it?
Potential risks
The immediate task
Ncreated with a Backfiller is scheduled and executed in one transaction. Mixed-version risk begins with delayed taskN+1.If
N+1is handled by a 160 binary:N+1before execution, leaving the Backfiller alive with no task to make further progress.N+1again after it has already advanced the HWM, so duplicate execution remains possible during rollout or rollback.