DanielLeens commented on PR #12081:
URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-5558572344
@nzw921rx thanks for this — I want to engage with it properly rather than
just restate my earlier "Ready to merge," because you've drawn a distinction I
glossed over.
**Where I agree, and think it matters for how this PR should land.** You're
right that this PR bundles two genuinely different questions, and my own review
conflated them more than it should have:
1. Are `RequestFuture`'s completion/success semantics and `WALWorkHandler`'s
catch-widening correct and worth having on their own? — I re-verified these
line-by-line against the `Future` contract and the Disruptor `WorkHandler`
semantics, and I stand by that part: the batch path's hard-coded 1-second cap
(vs. the configured 60s timeout) and the sole WAL consumer thread dying on any
non-`IOException` are real, independently-justifiable bugs regardless of what
#12058's root cause turns out to be.
2. Does collapsing the redundant `HdfsWriter.flush()` sync calls actually
explain and resolve the CV/variance reported in #12058? — On this, I did not
hold the evidence to the bar you're describing, and I should have. Concretely,
three gaps in what I accepted:
- The before/after benchmark evidence ran on different GitHub-hosted
runner CPUs, which @Rangsh already flagged as a confound — but I treated the
*directional* CV drop as meaningful signal rather than asking whether it
survives a same-machine, controlled comparison at all.
- The evidence bundles all three changes (sync collapse + `RequestFuture`
timeout fix + exception-handling fix) in one before/after pair, so there's no
way to attribute the CV change to the sync removal specifically rather than,
say, the batch path no longer silently truncating waits at 1s.
- Most importantly, as you point out: the benchmark exercises `file:///`
(Hadoop `LocalFileSystem`), and I never checked whether `LocalFileSystem`'s
`hflush()`/`hsync()` actually perform a real fsync-to-device on this path, or
whether removing one of two calls in that specific branch is closer to removing
a cheap redundant method call than removing real disk-sync latency.
Method-call-count parity (which is exactly what `HdfsWriterFlushSyncPathTest`
verifies) is not the same claim as disk-sync-count parity, and I conflated the
two when I described the test as confirming the fix's effect on the reported
variance.
**Where I think this leaves the PR.** I don't think the fix chain you're
asking for (baseline reproduction → profiling → hypothesis → isolated change →
controlled before/after → re-profile → durability check) is something this PR
needs to complete before its *correctness* fixes can land — those don't need a
CV improvement to justify themselves, exactly as you said. But I do think the
PR's own framing overstates what's been shown: it's opened with `Fixes #12058`,
which auto-closes that issue on merge, and the root-cause claim in the
description ("Root cause: ... increased latency and CV without improving
durability") is stated as established fact rather than as the working
hypothesis it currently is.
Concretely, I'd ask @Rangsh to:
- Drop or reword the `Fixes #12058` keyword (e.g. `Related to #12058`) so
merging this doesn't close an issue whose root cause is still under
investigation by your standard above.
- Soften the "Root cause" section to reflect that it's a hypothesis
supported by a wall-clock profile pointing at the sync path, not a confirmed,
isolated, same-machine controlled result.
That said, I don't think this needs to block the correctness fixes from
merging — they stand on their own, and #12058 can stay open and be investigated
on its own timeline using the process you outlined, unblocked by whether this
PR merges. Thanks for pushing on this; it's a better outcome than either of us
just leaving the variance claim unexamined.
--
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]
