fix(sandboxclaim): make claim batch size flag actually take effect#656
Conversation
The -sandboxclaim-max-batch-size flag was bound to a package variable (maxClaimBatchSize, default 10) that no code ever read. The real claim logic uses the core.MaxClaimBatchSize constant (50), so the flag was dead: setting it had no effect, and its advertised default (10) did not even match the effective value (50). Bind the flag directly to core.MaxClaimBatchSize (turned into a var), and add a matching -sandboxclaim-initial-batch-size flag for the exponential ramp-up start value (core.InitialClaimBatchSize). This lets operators tune claim throughput for large-scale deployments as intended, and removes the now-unused maxClaimBatchSize variable. Signed-off-by: liujiuyi <liujiuyi.ljy@alibaba-inc.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #656 +/- ##
==========================================
+ Coverage 80.04% 80.06% +0.01%
==========================================
Files 229 229
Lines 17861 17863 +2
==========================================
+ Hits 14297 14302 +5
+ Misses 2978 2976 -2
+ Partials 586 585 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Ⅰ. Describe what this PR does
The
-sandboxclaim-max-batch-sizeflag has never had any effect. It wasbound to a package variable
maxClaimBatchSize(default 10) that no codereads, while the claim logic uses the
core.MaxClaimBatchSizeconstant(50). So setting the flag did nothing, and its advertised default (10) did
not even match the effective value (50).
This PR:
-sandboxclaim-max-batch-sizetocore.MaxClaimBatchSize(turnedinto a var) so it actually takes effect.
-sandboxclaim-initial-batch-sizeto also tune the initial ramp-upsize (
core.InitialClaimBatchSize).maxClaimBatchSizevariable.No behavior change by default: the effective defaults stay 50 / 5. Only
the flag's advertised default is corrected (10 → 50).
6 insertions, 3 deletions across two files.
Ⅱ. Does this pull request fix one issue?
NONE. Found while trying to tune claim throughput and noticing the flag had
no effect.
Ⅲ. Describe how to verify it
-sandboxclaim-max-batch-size=100.The claim loop still batches at 50 (the constant) — the flag is ignored.
-sandboxclaim-initial-batch-sizelikewise controls the ramp-up start.
identical to before.
go build ./pkg/controller/sandboxclaim/... go test ./pkg/controller/sandboxclaim/...Ⅳ. Special notes for reviews
const→varfor the two batch sizes is required so they can be boundto flags via
flag.IntVar.in this repo passes the flag, so wiring it up cannot alter existing
behavior; only explicit user-provided values now take effect.
existing user could have depended on it working.