reiabreu commented on PR #8778:
URL: https://github.com/apache/storm/pull/8778#issuecomment-4624835723
Disclaimer: ran this through an LLM for thoroughness
Most of the following items are nice-to-haves, but can you check numbers 4
and 5?
Thank you
**1. 🟡 Medium — Missing comment on the null-guard in `DefaultScheduler` /
`EvenScheduler`**
```java
for (TopologyDetails topology : cluster.needsSchedulingTopologies()) {
if (topologies.getById(topology.getId()) == null) {
continue;
}
```
This guard is non-obvious — it exists because
`redistributeOntoIdleSupervisors` can call
`freeSlot` + `assign` on a topology that was previously "needs scheduling",
which causes its
snapshot to disappear from `Topologies` mid-loop. Worth a one-line comment
so future
contributors don't remove it as dead code.
**2. 🟢 Low — `hasIdleSupervisorReusableBy` re-reads config and re-iterates
supervisors on every call**
`redistributeOntoIdleSupervisors` already builds the idle-supervisor set and
checks the
feature flag. `hasIdleSupervisorReusableBy` then re-evaluates both for every
topology in the
per-topology budget loop, creating O(topologies × supervisors) redundant
work. It's
negligible at scale, but a simple cached `boolean enabled` and a
pre-computed idle-supervisor
set passed into the topology loop would avoid the duplication entirely.
**3. 🟢 Low — `SupervisorDetails` constructor count**
The class goes from 6 to 10 constructors. This is consistent with the
existing style, but a
static factory pair — `SupervisorDetails.of(...)` /
`SupervisorDetails.withUptime(...)` — or
a builder would make it easier to see which fields are being set. Not a
blocker, just worth
considering given 3.0.0 is a major release that can break APIs.
---
### Tests
**4. 🔴 High — Missing total-worker count assertions**
Several tests verify per-supervisor slot counts but don't assert that the
total number of
assigned workers is preserved. A concrete example of what slips through:
In `redistributeRelocatesAtMostMaxFreeWorkersPerTopology` the test asserts:
```java
assertEquals(1, usedSlotCount(cluster, "sup-0"));
assertEquals(1, usedSlotCount(cluster, "sup-1"));
assertEquals(1, usedSlotCount(cluster, "sup-2"));
```
Imagine a bug in `relocateOneWorkerOntoIdleSlot` where
`cluster.assign(target, ...)` is
called but the executor collection passed is empty (e.g. an erroneous `execs
== null` early
return leaves the target slot occupied but the executors unassigned). All
three
`usedSlotCount` assertions still pass — 3 slots are in use — but the
topology now has 3
workers with fewer executors than it declared. The bug is invisible.
Adding one line to every relocating test closes this entirely:
```java
// topology declared numWorkers=3; relocation must preserve that
assertEquals(3, cluster.getAssignedNumWorkers(firstTopology(cluster)));
```
Affected tests: `redistributeRelocatesAtMostMaxFreeWorkersPerTopology`,
`scheduleTopologiesEvenly_movesOneWorkerToIdleSupervisor`,
`evenDistributionInOneRound_unboundedMaxFree`, and the two
scheduler-integration tests.
**5. 🔴 High — Round-robin test under-asserts**
`multipleTopologies_shareIdleSlotsRoundRobin` is the only test that
exercises the core
round-robin fairness guarantee, but its assertions don't actually enforce
it. The test
currently checks something like:
```java
assertTrue(slotsOnSup2ForTopoA >= 1);
assertTrue(slotsOnSup2ForTopoB >= 1);
```
Consider a broken implementation where the inner topology loop exits after
the first
successful topology instead of continuing round-robin. `topo-A` (sorted
first by id) would
consume all 4 idle slots on sup-2; `topo-B` gets zero. Yet if sup-2 also
happens to have
`topo-A`'s workers already placed there via a previous iteration, the `>= 1`
check still
passes — the round-robin bug is completely invisible.
The fix is to assert exact counts and verify worker totals per topology:
```java
// sup-2 has 4 slots; with 2 topologies, round-robin gives each exactly 1
slot
// (each topology's budget = floor(4 workers / 3 supervisors) * 1 idle = 1)
assertEquals(1, slotsOnSup2ForTopoA);
assertEquals(1, slotsOnSup2ForTopoB);
// Total worker count must be preserved for both topologies
assertEquals(4, cluster.getAssignedNumWorkers(topoA));
assertEquals(4, cluster.getAssignedNumWorkers(topoB));
// No supervisor should have been drained to zero
assertTrue(usedSlotCount(cluster, "sup-0") > 0);
assertTrue(usedSlotCount(cluster, "sup-1") > 0);
```
Without these, the test is really only checking "something happened on
sup-2", not that
round-robin sharing works.
**6. 🟡 Medium — Flap-guard boundary coverage**
Tests `flapGuard_belowThreshold_doesNotMove` and
`flapGuard_atThreshold_moves` cover
`uptime < threshold` and `uptime == threshold` as two separate methods. A
`@ParameterizedTest`
with `threshold - 1`, `threshold`, and `threshold + 1` would give stronger
boundary coverage
with less code and make the off-by-one contract explicit.
**7. 🟢 Low — `noIdleSupervisor_doesNotTrigger` conf inconsistency**
This test hand-builds its conf map and omits
`NIMBUS_EVEN_REBALANCE_IDLE_SUPERVISOR_MIN_STABLE_ROUNDS`,
while every other test goes through `evenRebalanceConf(...)`. The test
passes because
`genSupervisors` defaults to `Long.MAX_VALUE` uptime, but the inconsistency
makes it harder
to trust at a glance. Routing it through the shared helper would make the
intent clearer.
**8. 🟢 Low — Setup duplication**
`noIdleSupervisor_doesNotTrigger`, `redistributeNeverDrainsSupervisorToZero`,
`singleWorkerTopology_doesNotMoveDespiteIdleSupervisors`, and
`evenDistributionInOneRound_unboundedMaxFree` each inline 15–25 lines of
slot/executor
wiring that is almost identical to `buildClusterWithIdleSupervisor`.
Extracting a small
fluent `ClusterBuilder` test helper (or at least a shared
`buildAssignment(topology, slots[])`
method — which `multipleTopologies_shareIdleSlotsRoundRobin` already has!)
would halve the
boilerplate.
--
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]
