mwkang commented on PR #8778: URL: https://github.com/apache/storm/pull/8778#issuecomment-4627327528
Thanks for the thorough review — and for flagging 4 and 5 specifically; those were the right things to tighten. I've pushed a follow-up addressing the feedback. Item by item: **4 (total-worker assertions)** — Done. Every relocating test now asserts `cluster.getAssignedNumWorkers(topology)` against the declared `numWorkers`: `redistributeRelocatesAtMostMaxFreeWorkersPerTopology`, `scheduleTopologiesEvenly_movesOneWorkerToIdleSupervisor`, `evenDistributionInOneRound_unboundedMaxFree`, and both scheduler-integration tests — plus `donorTieBreaksBySupervisorIdWhenWorkerCountsTie`, which was the one relocating test I'd missed. This is a genuinely independent check: `getAssignedNumWorkers` counts distinct slots in the executor→slot map, whereas `usedSlotCount` reads `nodeToUsedSlotsCache`, so an `assign(target, <empty execs>)` bug now fails the count assertion even though the slot still registers as used. **5 (round-robin under-asserts)** — Done. `multipleTopologies_shareIdleSlotsRoundRobin` now asserts exact counts rather than `>= 1`: `usedSlotCount(sup-2) == 2` with `supervisorWorkerCount == 1` for each of topo-A and topo-B, `getAssignedNumWorkers == 4` for both, and that neither donor (sup-0/sup-1) is drained to zero. I confirmed the new assertions actually catch the failure mode by temporarily breaking the inner loop so the first topology grabs both idle slots — the test then fails on topo-B's count, as intended. **1 (null-guard comment)** — Added a comment above the guard in both `DefaultScheduler` and `EvenScheduler`. The rationale turned out to be slightly different from the one in your note: `needsSchedulingTopologies()` returns the cluster's *full* topology set, while each caller scopes the run to the topologies it passed in (the full set from `*.schedule`, where the guard is a no-op; a single-topology `Topologies` per leftover from `DefaultScheduler.defaultSchedule`; the leftover non-isolated subset from `IsolationScheduler`). The guard skips anything outside that set so the leftover path never schedules a topology the caller excluded — e.g. a down isolated topology on a reserved host. The comment now spells out those cases. **2 (redundant config re-read / supervisor rescan)** — Done. The feature-flag check is hoisted to the top of `redistributeOntoIdleSupervisors` (early return), and the idle-supervisor set is computed once up front and passed into a new private `topologyCanReuseIdleSupervisor(...)` helper, replacing the per-topology `cluster.hasIdleSupervisorReusableBy(...)` call. It's behaviorally equivalent — same `getUsedSlotsByTopologyId` existence check against the idle set — but the config read and full supervisor scan now happen once per round instead of once per topology. (`Cluster.hasIdleSupervisorReusableBy` stays as the public binary-trigger oracle the tests assert against.) **6 (flap-guard boundary)** — Done. The two methods are merged into a single `@ParameterizedTest` (`flapGuardHonorsMinStableRoundBoundary`) with `@CsvSource` covering `threshold-1 / threshold / threshold+1` (8→no-move, 9→move, 10→move at `minStableRounds=3`, `monitorFreq=3`), asserting both the trigger boolean and the resulting placement. Makes the `uptimeSecs >= minStableRounds * monitorFrequencySecs` contract explicit. **7 (conf inconsistency)** — Done. `noIdleSupervisor_doesNotTrigger` now goes through the shared `evenRebalanceConf(...)` / `newCluster(...)` helpers, and I swept up the last remaining hand-built fixture (`multipleTopologies_shareIdleSlotsRoundRobin`) so every test uses the same path. **8 (setup duplication)** — Done. Extracted a shared `buildAssignment(topology, slots[])` helper; the four tests you listed now use it together with `newCluster`/`evenRebalanceConf`, which removes the bulk of the inline slot/executor wiring. **3 (SupervisorDetails constructor count)** — Left as constructors for now. The new `uptimeSecs` overloads chain through to existing defaults (`Long.MAX_VALUE`), so every current caller is unchanged, and the style matches the rest of the class. A static factory / builder is a reasonable cleanup, but it's a public-API change that's somewhat orthogonal to this PR — I'd prefer to keep it out of scope here and do it as a separate follow-up if you feel it's worth it. Let me know. Thanks again! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
