aglinxinyuan commented on PR #8177: URL: https://github.com/apache/texera/pull/8177#issuecomment-5479746104
Thanks for the review — four of the five applied, one refused, plus one thing the comments didn't catch. Pushed as `be53e5c4` and `1ce599bd`. **Applied.** - **`ReplayLogWriterFixtures:154`** (swallowed `Throwable`) — `terminatesWithin` now stores what `shutdown()` threw in an `AtomicReference` and rethrows the original instance on the calling thread once `join()` returns. Renamed the thread local `t` → `shutdownThread` so the catch binding no longer shadows it. - **`:155`** (wedged shutdown leaves the thread running) — after `join(timeoutMillis)` it computes `returned = !shutdownThread.isAlive`, calls `interrupt()` when the budget elapsed, and only consults the captured throwable when the thread actually returned. - **`AsyncReplayLogWriterSpec:213`** (hard-coded `250L`) — added a named `sleepGranularityToleranceMillis = 50L` and derived the bound as `flushIntervalMillis - tolerance`. The failure message now names both the floor and the interval it came from. - **`CongestionControlSpec:191`** (full-string `getStatusReport` assertion) — premise accepted, remedy adjusted. The clamp test now asserts the `canSend` boundary directly (`getInTransitMessages.isEmpty` and `canSend`, then `markMessageInTransit` and `!canSend`, i.e. `windowSize == 1` exactly, twice). `getStatusReport` is still built, but as the failure clue rather than the assertion. **Refused: `ReplayLogWriterFixtures:120`, the fixed `Thread.sleep` in `close()`.** That sleep isn't incidental latency — it's the device that makes the durability ordering decidable. `AsyncReplayLogWriter.run()` closes the record writer and only *then* completes the future `terminate()` waits on, and the fixture snapshots the trace on the shutdown thread the instant `terminate()` returns. Without a delay in `close()`, "did the writer finish flushing before it signalled completion?" stops being an observable question and the ordering assertions become vacuous rather than fast. A latch would let the fixture *observe* the ordering but not *require* it. **One thing the review didn't surface, found while re-verifying.** The dedicated `getStatusReport` format test drove all three counters to `1` — `current window size = 1 \t in transit = 1 \t waiting = 1`. A string in which every field holds the same value can't distinguish the fields from one another, so a mutant transposing two of them survived. The counters are now driven to three distinct values (`2 / 1 / 3`: one in-window ack doubles the window via slow start, one `markMessageInTransit`, three `enqueueMessage`), which is what the comment's underlying concern was really pointing at. 33/33 tests pass, lint clean, production diff empty, and the no-hang property of the shutdown helper is preserved — every test still starts before terminating, with `terminate()` in a `finally`. -- 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]
