rangareddy opened a new pull request, #19485:
URL: https://github.com/apache/hudi/pull/19485
### Describe the issue this Pull Request addresses
Relates to #16228 (HUDI-6843), a flaky
`testUpsertsContinuousModeWithMultipleWritersForConflicts`. Open
since September 2023, and every report of it is this and nothing more:
```
[ERROR]
testUpsertsContinuousModeWithMultipleWritersForConflicts{HoodieTableType}[1]
<<< ERROR!
java.util.concurrent.TimeoutException
at java.util.concurrent.FutureTask.get(FutureTask.java:205)
at
HoodieDeltaStreamerTestBase$TestHelpers.waitTillCondition(HoodieDeltaStreamerTestBase.java:650)
at
TestHoodieDeltaStreamer.deltaStreamerTestRunner(TestHoodieDeltaStreamer.java:737)
```
**This PR does not stop the test flaking.** It makes the next occurrence
diagnosable, which is the
prerequisite for fixing it — and is why the issue has gone three years
without progress.
`waitTillCondition` polls the condition every two seconds and swallows
whatever it throws:
```java
} catch (Throwable error) {
log.debug("Got error waiting for condition", error);
ret = false;
}
```
The condition for this test asserts four things — delta-commit count,
compaction-commit count, record
count and distance count. When one of them never becomes true, the assertion
error goes to `debug` and is
dropped, `res.get(360, SECONDS)` expires, and the failure names only the
helper. There is no way to tell
which assertion was still failing, or what value it saw, so no two reports
of this flake can be
distinguished and none of them is actionable.
### Summary and Changelog
- `waitTillCondition` keeps the last `Throwable` the condition threw and
attaches it to the failure, so a
timeout reports what it was still waiting for instead of only that it gave
up.
- It no longer logs `"Condition completed successfully"` when the condition
returned **false** — that line
fired on every unsuccessful poll, which actively misleads anyone reading
the log of a flake.
- It shuts down the polling executor. `Executors.newSingleThreadExecutor()`
was created per call and never
shut down, leaking a thread on every wait; every continuous-mode
deltastreamer test goes through here.
`InterruptedException` is caught ahead of the catch-all so the shutdown
actually stops the thread — see
the note under Verification for why the catch-all alone would not.
- `deltaStreamerTestRunner` now consumes `dsFuture` if it has already
finished, before calling
`awaitDeltaStreamerShutdown`. That branch never consumed the future, so a
streamer that died was
reported two minutes later as `"Deltastreamer should have shutdown by
now"`, hiding the real cause. The
other branch already does this via `dsFuture.get()`.
### Verification
New `TestWaitTillCondition` covers the helper directly, without Spark:
| test | what it pins |
| --- | --- |
| `timeoutFailureNamesTheLastConditionFailure` | the timeout failure carries
the condition's own error text |
| `satisfiedConditionReturnsNormally` | the happy path still returns |
| `finishedStreamerEndsTheWaitWithoutFailing` | a finished streamer still
ends the wait without failing, so the new timeout handling does not turn that
into a failure |
| `pollingStopsOnceTheWaitHasGivenUp` | the polling thread actually stops
when the wait gives up — added in review, see below |
Reverting the helper to master's form makes the first test fail with exactly
the symptom from the issue,
which is the evidence that it is testing the right thing:
```
AssertionFailedError: Unexpected exception type thrown,
expected: <java.lang.AssertionError> but was:
<java.util.concurrent.TimeoutException>
```
After the change the same scenario reports:
```
Condition was not met within 15 seconds. The last failure it reported was:
java.lang.AssertionError: assertAtleastNDeltaCommits: expected at least 3 delta
commits but got 2
```
The condition's error is also set as the failure's cause, so the original
stack trace survives.
4 tests green (27.4s), `checkstyle:check` and `apache-rat:check` clean
(`Unapproved: 0`).
**Why `InterruptedException` is caught separately** (raised in review, and
the reason for the second commit
that is now squashed in): catching it with the catch-all makes the executor
shutdown close nothing.
`shutdownNow()` interrupts the polling thread, but `Thread.sleep` clears the
interrupt flag when it throws, so
the catch-all swallowed the `InterruptedException` and re-entered the loop.
`dsFuture` never completes in the
timeout case, so the loop had no other exit and the thread kept polling for
the life of the JVM — in exactly
the case the shutdown was added for. `pollingStopsOnceTheWaitHasGivenUp`
pins this: it counts evaluations
after the wait fails, and with the interrupt handling reverted it reports 4
polls where 2 were expected.
The timeout in `timeoutFailureNamesTheLastConditionFailure` was widened from
3s to 15s for the same review
round. At one poll interval of slack, a late worker start could leave
nothing recorded to report, which would
make a de-flaking test itself timing-sensitive.
**The target test itself:**
`testUpsertsContinuousModeWithMultipleWritersForConflicts` passes with these
changes, run four times over both `HoodieTableType` parameters — 8 green
executions, ~88s per run:
```
run 1: Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
run 2: Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
run 3: Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
run 4: Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
```
The full `TestHoodieDeltaStreamerWithMultiWriter` class also passes — re-run
on the squashed commit: 5 tests,
0 failures, 107.5s. No flake reproduced in those runs, so this change is not
masking one — and had it flaked,
the new failure would have named the assertion that was still failing, which
is the whole point.
### Impact
Test infrastructure only — no production code. Nothing that passes today
starts failing: the only new
failure path is the timeout, which already failed, and the "streamer already
finished" case is pinned by a
test specifically to keep it non-failing.
### Risk Level
low — test-only, and the behaviour changes are limited to what a failure
reports.
### Documentation Update
none
### Contributor's checklist
- [x] Read through [contributor's
guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
- [x] CI passes on my PR
--
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]
