review: post-merge round over the batch, with a fix for the plural node read - #748
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix (own commit, regression test in
TestGetNodesResourceInfo)cpumemread the requested nodes with one strictGetMulti, so a node whose record was gone (reachable after a crash between the plugin's and the store'sRemoveNode) 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
/simplifylenses 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:ControlWorkloaddispatches through one verb table instead of an entry guard and a switch that repeated the list with a dead arm.SetWorkloadsStatusreturns the metas it was given instead of an element-wise copy.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).FillPlantests the map it builds instead of a running sum; the plugin cache is keyed once; the binary client leaves the verb check tocall.sshrunner.Exited, whichStreamnow shares.NodeResourceRequestwhere 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:
nodeErrin 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 infilterNodes(20+ calcium tests pinGetNode); 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-checkandaslon both GOOS green.