Skip to content

review: post-merge round over the batch, with a fix for the plural node read - #748

Merged
CMGS merged 2 commits into
masterfrom
review/round4
Sep 9, 2026
Merged

CMGS merged 2 commits into
masterfrom
review/round4

Conversation

@CMGS

@CMGS CMGS commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Fix (own commit, regression test in TestGetNodesResourceInfo)

cpumem read the requested nodes with one strict GetMulti, so a node whose record was gone (reachable after a crash between the plugin's and the store's RemoveNode) failed the whole listing's cpumem column; the read now falls back to one lookup per key and leaves the missing node out, which the interface already promised. Cobalt no longer seeds an entry for every requested node, so a node no plugin knows stays untouched instead of reading as zero, and it asks no plugin at all for an empty list, which spawned every binary plugin only to log an error on an empty or filtered pod.

Review

The four /simplify lenses over the whole batch since the last style round, plus a style and a judge read of every file the batch changed (16 + 12 files, read in full). Applied:

  • ControlWorkload dispatches through one verb table instead of an entry guard and a switch that repeated the list with a dead arm.
  • SetWorkloadsStatus returns the metas it was given instead of an element-wise copy.
  • cpumem plans capacity through one bounded loop (limit 1 unless CPUBind) instead of two copies of the loop, and decodes the plural read straight from the stored blob (one JSON pass instead of three, on every listing).
  • FillPlan tests the map it builds instead of a running sum; the plugin cache is keyed once; the binary client leaves the verb check to call.
  • The two engines' identical session drains become sshrunner.Exited, which Stream now shares.
  • Binary request types name NodeResourceRequest where they carry one; three restating comments go, two field docs become sentences (comment delta +4 −7); the four repeated cpumem node tests are tables.

Rejected, with the reason recorded in the round ledger: nodeErr in realloc (the shadow form fails govet, the gate caught it once); byLockOrder's prefix rank (derived from the same format that builds the keys); the single-include branch in filterNodes (20+ calcium tests pin GetNode); the fallback fan-out bound of 16 (lane A/B at 50 nodes: 619 vs 623 ms against the unbounded old path).

Lines

Production −14 net, tests +72 net (the tables carry subtest names).

Evidence

Gate on the branch: build, vet, full tests, make lint, make fmt-check and asl on both GOOS green.

CMGS added 2 commits September 9, 2026 17:48
cpumem read the requested nodes with one strict GetMulti, so a node
whose record was gone failed the whole listing's cpumem column; the
read now falls back to one lookup per key and leaves the missing node
out, which the interface already promised. Cobalt no longer seeds an
entry for every requested node, so a node no plugin knows stays
untouched instead of reading as zero, and it asks no plugin at all for
an empty list, which spawned every binary plugin only to log an error
on an empty pod.
ControlWorkload dispatches through one verb table instead of a guard
list and a switch that repeated it; SetWorkloadsStatus returns the
metas it was given instead of an element-wise copy; cpumem plans
capacity through one bounded loop and decodes the plural read straight
from the stored blob (one JSON pass instead of three); FillPlan tests
the map it builds instead of a running sum; the plugin cache is keyed
once; the binary client leaves the verb check to call; the two engines'
session drains become sshrunner.Exited, which Stream now shares; the
binary request types name NodeResourceRequest where they carry one;
three restating comments go and two field docs become sentences; the
cpumem node tests are tables.
@CMGS
CMGS merged commit d1f0ce2 into master Sep 9, 2026
7 checks passed
@CMGS
CMGS deleted the review/round4 branch September 9, 2026 10:02
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