Push maintenance leader startup logic down into maintenance package#1196
Push maintenance leader startup logic down into maintenance package#1196
Conversation
brandur
force-pushed
the
brandur-push-maintainer-logic
branch
from
5326f39 to
4d86b90
Compare
|
@codex review |
![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: 4d86b90c44
ℹ️ 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".
| return leader | ||
| } | ||
|
|
||
| t.Run("StartsMaintainerOnLeadershipGain", func(t *testing.T) { |
There was a problem hiding this comment.
/workspace/river/AGENTS.md requires inner t.Run blocks to be sorted alphabetically, but StartsMaintainerOnLeadershipGain is declared before RetriesAndResignsOnStartFailure. This violates the repo’s documented test-organization rule for newly added tests and makes future maintenance/diff review less predictable; swap the two subtest blocks to keep the suite compliant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hah, this is the first time I've seen it try to enforce these on review! We could definitely make the AGENTS.md rule more nuanced to allow for some flexibility/discretion here, feel free to adjust as you'd like it to be.
There was a problem hiding this comment.
Ah right! Let me rewrite this. Oddly, I would've expected my local Claude to have caught it by virtue of being in agents, but guess not.
| // RequestResignFunc is a function that sends a notification requesting leader | ||
| // resignation. It's injected from the client because the notification mechanism | ||
| // depends on the driver, which the maintenance package doesn't know about. | ||
| type RequestResignFunc func(ctx context.Context) error |
There was a problem hiding this comment.
/workspace/river/AGENTS.md states that types added to new code should be alphabetized, but RequestResignFunc is declared before QueueMaintainerLeaderConfig/QueueMaintainerLeader. Keeping declarations out of the mandated order increases scan time and drifts from the repository’s required structure conventions; reorder the type blocks to match alphabetical order.
Useful? React with 👍 / 👎.
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Solid cleanup, thanks! 🙌
| // RequestResignFunc is a function that sends a notification requesting leader | ||
| // resignation. It's injected from the client because the notification mechanism | ||
| // depends on the driver, which the maintenance package doesn't know about. | ||
| type RequestResignFunc func(ctx context.Context) error |
There was a problem hiding this comment.
thoughts on ditching the type in favor of just inlining the description and func(context.Context) error on the field down below?
There was a problem hiding this comment.
Oh my god, this was an AI-ism that emerged when I asked it to move the implementation. Removed!
There was a problem hiding this comment.
My bad, should've caught that one.
brandur
force-pushed
the
brandur-push-maintainer-logic
branch
2 times, most recently
from
1444165 to
74af83b
Compare
This one's a quick follow up to #1184. Unfortunately in order to cover all the edge cases that Codex found during review we ended up with fairly complex start logic for the queue maintainer because it needs to have a separate goroutine, do a lot of context checking, and track its current leadership "epoch" to make sure that an older start attempt doesn't affect the start attempt booted off by a newer leadership signal. This had left an unfortunate amount of code in `client.go`. Here, break this all out into its own file in `internal/maintenance/`. This lets us streamline `Client` back to a nicer point.
brandur
force-pushed
the
brandur-push-maintainer-logic
branch
from
74af83b to
daf43be
Compare
|
Race tests found one more intermittently failing test case, but fixed now! Thanks. |
2 participants
This one's a quick follow up to #1184. Unfortunately in order to cover
all the edge cases that Codex found during review we ended up with
fairly complex start logic for the queue maintainer because it needs to
have a separate goroutine, do a lot of context checking, and track its
current leadership "epoch" to make sure that an older start attempt
doesn't affect the start attempt booted off by a newer leadership
signal. This had left an unfortunate amount of code in
client.go.Here, break this all out into its own file in
internal/maintenance/.This lets us streamline
Clientback to a nicer point.