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.
- `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, in ~6s, 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 |
Reverting the helper to its current 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 3 seconds. The last failure it reported was:
java.lang.AssertionError: assertAtleastNDeltaCommits: expected at least 3
delta commits but got 2
```
3 tests green, `checkstyle:check` and `apache-rat:check` clean.
**What I could not verify locally:** the full
`testUpsertsContinuousModeWithMultipleWritersForConflicts`
run. It fails on my machine before reaching any of this, with
`NoClassDefFoundError:
org/apache/spark/sql/internal/SQLConf$LegacyBehaviorPolicy$` from
`HoodieSpark3_5AvroDeserializer` — a Spark version mismatch in my local
artifacts, unrelated to this
change. The two behavioural paths of the helper are covered by the unit
tests above; the three-line
`dsFuture` change in the runner is only reachable when the streamer has
already failed. CI exercises the
real test.
### 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]
