Skip to content

Rename ExecuteOptions.unique_id to loadshed_valve_id#893

Open
bgwines wants to merge 1 commit into
bwines/snake-22from
bwines/snake-loadshed-valve-id-rename
Open

Rename ExecuteOptions.unique_id to loadshed_valve_id#893
bgwines wants to merge 1 commit into
bwines/snake-22from
bwines/snake-loadshed-valve-id-rename

Conversation

@bgwines

@bgwines bgwines commented Jun 26, 2026

Copy link
Copy Markdown

Background / Why?

The Snake load shedder keys its per-valve self-contention layer on a field of ExecuteOptions, but that field was named unique_id while both the Snake RFC and the loadshed package call the concept a "valve ID". The mismatch made the wiring confusing to follow — call sites read GetUniqueId() and immediately passed the result into Acquire(ctx, valveID, ...), so a reader had to know that unique_id was the valve ID. The RFC specifies the field should be named loadshed_valve_id.

This renames the proto field unique_idloadshed_valve_id. Field number 22 is unchanged, so the wire format stays backwards-compatible; only the Go symbol and JSON name change. ExecuteOptions is forwarded from vtgate to vttablet wholesale, so there is no per-field passthrough to update — the only readers are the two Snake acquire sites (QueryExecutor.getConn and TxPool.Begin), which now call GetLoadshedValveId().

While here, the "an empty valve ID is valid" explanation that previously sat at both call sites is moved into Snake.Acquire's doc comment, where the contract actually lives, so it is not duplicated and is visible to any future caller.

The generated .pb.go files were regenerated with the same protoc-gen-go version already used for the checked-in code (v1.36.11), so the diff is limited to the rename rather than a toolchain-version reformat.

Testing

Updated the existing Snake unit tests to use the renamed field (LoadshedValveId in ExecuteOptions literals) and ran go test for go/vt/vttablet/tabletserver and go/vt/vttablet/tabletserver/loadshed. go build ./go/vt/... confirms the rename is consistent across the tree (the only non-generated readers are the two acquire sites).

AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :)

The Snake load shedder uses this ExecuteOptions field as its per-valve
contention key, but the field was named unique_id while the RFC and the
loadshed package both call the concept a "valve ID". The mismatch made the
wiring hard to follow: callers read GetUniqueId() and passed the result into
Acquire(ctx, valveID, ...).

Rename the proto field unique_id -> loadshed_valve_id (field number 22 is
unchanged, so the wire format is backwards-compatible) and update the two
reader call sites and their tests to match. ExecuteOptions is forwarded from
vtgate to vttablet wholesale, so no per-field passthrough wiring is needed.

Also relocate the "empty valve ID is valid" explanation from the two call
sites into Acquire's doc comment, where the contract actually lives.

AI disclosure: Claude Code assisted with development. Every line of code was either written by or carefully reviewed by me :)

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Brett Wines <bwines@slack-corp.com>
@github-actions github-actions Bot added this to the v22.0.4 milestone Jun 26, 2026
@bgwines bgwines marked this pull request as ready for review June 26, 2026 17:20
@bgwines bgwines requested a review from a team as a code owner June 26, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant