fix: Precedence of taskCount, taskCountStart, taskCountMin.#19417
Conversation
PR apache#18745 included a discussion of desired behavior of taskCount, taskCountStart, and taskCountMin, but this has not been fully implemented. There are two missed cases: 1) On OL / supervisor restart, the persisted taskCount should be used. Currently, the persisted taskCountStart is used instead if set. 2) On supervisor POST, the previous spec's taskCount should be used if the user provides neither taskCount nor taskCountStart. Currently, in this case, the user's provided taskCountMin is used instead. In addition, and related to (1), currently deserialization of the ioConfig causes the taskCount to be overwritten by the taskCountStart. This causes the get supervisor and supervisor history APIs to return inaccurate taskCounts. This patch fixes these issues by doing two things: 1) Update SeekableStreamSupervisorIOConfig to always retain taskCount from the constructor (i.e. when deserializing) if set. 2) Update SeekableStreamSupervisorSpec#merge to handle the special case that taskCountStart has priority on POST.
0c8b706 to
159f227
Compare
gianm
changed the title
gianm
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (autoScalerEnabled) { | ||
| // Priority: taskCountStart > taskCount > taskCountMin | ||
| this.taskCountExplicit = taskCount != null; | ||
| if (taskCount != null) { |
There was a problem hiding this comment.
nit: may use this.taskCountExplicit for condition
| this.taskCount = Configs.valueOrDefault(taskCount, autoScalerConfig.getTaskCountMin()); | ||
| } else { | ||
| this.taskCount = Configs.valueOrDefault(taskCount, 1); | ||
| this.taskCount = 1; |
There was a problem hiding this comment.
Why this change? Autoscaler also may be disabled, but spec may not be null (available).
Isn't it better to assign 1 only as a last resort?
AFAIR, this LOC in this verrsion led to some inaccurate behaviour some time ago, but do not remember exactly what it was. Anyway it is covered by case above.
There was a problem hiding this comment.
The question here is that if taskCount is not provided, and an autoscaler is provided yet disabled, what should we use for the initial taskCount? This only matters on the very first submission of a supervisor, since if there's an existing supervisor, we'll use its persisted taskCount (which will always be there).
Anyway, the old code used taskCountMin (ignoring taskCountStart if present). I thought that was strange: why use the autoscaler configs in a slightly different way if the autoscaler is not enabled? This PR changes it to use 1, on the grounds that if the autoscaler is submitted in a disabled state then its configuration should not apply.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.
This is an automated review by Codex GPT-5
…9417) PR apache#18745 included a discussion of desired behavior of taskCount, taskCountStart, and taskCountMin, but this has not been fully implemented. This patch updates logic to align with the intent.
4 participants
PR #18745 included a discussion of desired behavior of taskCount, taskCountStart, and taskCountMin, but this has not been fully implemented. There are two missed cases:
On OL / supervisor restart, the persisted taskCount should be used. Currently, the persisted taskCountStart is used instead if set.
On supervisor POST, the previous spec's taskCount should be used if the user provides neither taskCount nor taskCountStart. Currently, in this case, the user's provided taskCountMin is used instead.
In addition, and related to (1), currently deserialization of the ioConfig causes the taskCount to be overwritten by the taskCountStart. This causes the get supervisor and supervisor history APIs to return inaccurate taskCounts.
This patch fixes these issues by doing two things:
Update SeekableStreamSupervisorIOConfig to always retain taskCount from the constructor (i.e. when deserializing) if set.
Update SeekableStreamSupervisorSpec#merge to handle the special case that taskCountStart has priority on POST.