joseluisll opened a new pull request, #8682: URL: https://github.com/apache/hadoop/pull/8682
### Description of PR JIRA: [HADOOP-19964](https://issues.apache.org/jira/browse/HADOOP-19964) A test that fails on timeout now produces exactly one full JVM thread dump, with deadlock analysis, taken while the threads are still hung — in every module, with no per-pom wiring. That is the diagnostic you actually want for a hang, and today you get either nothing or two copies of it. Two things had to change to get there. `TimedOutTestsListener`, which prints the dump, has been dead code since February 2025 and is rewritten here as a JUnit Platform listener. And `GenericTestUtils.waitFor` — at 650 call sites the most common timeout in the suite — used to inline its own 16 KB dump into every `TimeoutException` message; it now routes through that same listener, so the message drops to one line and the dump joins the others on stderr. Scope of the change: 12 files, +336/-86, of which 54 deleted lines are dead pom configuration. This diagnostic has been silently dead since February 2025. HADOOP-19415 Part 4 moved Surefire onto the JUnit Platform provider, and `TimedOutTestsListener` was a JUnit 4 `RunListener` registered through the Surefire `listener` provider property — which that provider ignores. Eight poms have been carrying the property ever since, registering nothing, and a timed-out test has reported only its exception. The failure mode is invisible: a missing dump looks exactly like a test that never hung. **What changed** * `TimedOutTestsListener` is now a JUnit Platform `TestExecutionListener`, registered through `META-INF/services/org.junit.platform.launcher.TestExecutionListener` in the hadoop-common test resources. Because it rides in the hadoop-common test-jar, it activates anywhere that artifact is on the test classpath. That is a wider net than the old wiring ever cast, even when it worked: coverage no longer depends on each module remembering to paste a property. It detects JUnit 5 `@Timeout`, the JUnit 4 vintage runner's `TestTimedOutException` (by class name, so no vintage dependency), and any failure whose message says it timed out. * Two controls the 2012 original never had: `-Dhadoop.test.timedout.dump=false` disables the dump entirely, and `-Dhadoop.test.timedout.dump.limit` (default 5) caps the dumps one JVM prints, with a single elision notice at the cap. * One dump per timeout, in one place. `GenericTestUtils.waitFor` built a full thread dump into every `TimeoutException` it threw — some 16 KB per failure, at 650 call sites across 258 files. It now prints that dump through `TimedOutTestsListener.dumpForTimeout` and its message shrinks to one line saying where the dump went; the listener matches that marker and stays quiet. The two properties above now cover `waitFor` as well, which the inlined dump obeyed neither of. * The dead `listener` property is gone from all eight poms (hadoop-common, hadoop-kms, hadoop-hdfs, hadoop-hdfs-httpfs, hadoop-mapreduce-client-nativetask, hadoop-mapreduce-client, hadoop-mapreduce-project, hadoop-yarn); no pom in the tree references it any more. Where removal left an empty `<configuration/>`, that element is dropped but the plugin declaration is kept, so no module's surefire activation or version resolution changes. **Why `waitFor` still takes its own dump** rather than deferring to the listener: a `TestExecutionListener` is only notified at `executionFinished`, which fires once the test method *and its teardown* have unwound. For a hang, the threads you need to see are usually gone by then — a MiniDFSCluster has been shut down in `@AfterEach`. Dumping inside `waitFor` keeps the capture at the instant the wait expired, exactly where it was before this patch. The listener remains the only option for `@Timeout`, which gives no earlier hook. Both paths share the enable switch and the per-JVM budget. **Scope and limits** (verified against Surefire 3.5.3): the listener fires for timeouts that fail *through JUnit* (`@Timeout` and friends). It cannot cover a fork killed by Surefire at `forkedProcessTimeoutInSeconds` — `ForkClient#tryToTimeout` sends the fork `Shutdown.KILL` regardless of the configured shutdown strategy, and the fork executes `Runtime.halt()`, which bypasses listeners and shutdown hooks alike. That case is handled by HADOOP-19950 (Surefire dumpstream capture and CI upload). The two are complementary: this listener writes to `System.err`, so its dump lands in `surefire-reports/*-output.txt`, already inside the globs HADOOP-19950 uploads in CI. **Note for downstream consumers**: the same ServiceLoader registration that removes the per-pom wiring also means projects consuming the hadoop-common test artifact (HBase, Ozone, Hive, Tez, …) pick the listener up without asking for it, and `waitFor`'s exception message changes shape for them too — the dump moves out of the message and onto stderr. Nothing in the Hadoop tree asserted on that message, but downstream code that did will need adjusting. `-Dhadoop.test.timedout.dump=false` opts out of the dumps entirely. A release note is attached to the JIRA. This patch was developed with AI assistance. Contains content generated by Claude Code. ### How was this patch tested? * End-to-end through the real ServiceLoader path, under a plain `mvn test`: a scratch test (not part of this PR) pairing a hanging `@Timeout(3)` method with a `GenericTestUtils.waitFor` timeout. Each produced exactly one dump in `surefire-reports/*-output.txt`, labelled `Test: testHangOnJunitTimeout()` and `Timed out in: GenericTestUtils.waitFor` respectively, and the `@Timeout` case was confirmed in both `SAME_THREAD` and `SEPARATE_THREAD` modes. The `waitFor` message dropped from ~16 KB to a single line, taking its report file from 18,223 to 1,642 bytes; it now reads in full: `TimeoutException: Timed out waiting for condition. Thread dump printed to stderr.` The dump is moved, not discarded — it lands in `-output.txt` with the others, so total bytes are about the same. The win is that the failure you read first is legible. * `TestTimedOutTestsListener` (5 tests): thread-dump content and deadlock detection (6-thread monitor + synchronizer deadlock), timeout-failure detection, the `hadoop.test.timedout.dump=false` kill switch, the per-JVM dump limit and its single elision notice, and both halves of the `waitFor` change — that it prints its own dump with the right label and that its message is now one line, plus that the off switch reaches it. The `waitFor` tests drive the real helper, so they fail if either side changes. All pass, as does `TestGenericTestUtils` unchanged (13 tests together): `mvn -B test -pl hadoop-common-project/hadoop-common -Dtest='TestTimedOutTestsListener,TestGenericTestUtils'` * Registration verified to ship: the services file is copied into `hadoop-common/target/test-classes/META-INF/services/`, i.e. into the test-jar, alongside Hadoop's existing service registrations. The same run logs `Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider` — which is why the old JUnit 4 `listener` property registered nothing. * `mvn test-compile` passes on all eight modules whose poms changed. Because the `GenericTestUtils` change ships inside the hadoop-common test-jar, `hadoop-registry` — a consumer of that artifact — was also test-compiled against it: BUILD SUCCESS, so test-jar consumers are unaffected at compile time. * Checkstyle: `TimedOutTestsListener.java` and `TestTimedOutTestsListener.java` are both clean. The test file carried two violations inherited from the 2012 original — a package-private field in the `Monitor` helper and a brace-less `if` — which this PR fixes in passing, since it rewrites that file anyway. `GenericTestUtils.java` has pre-existing violations, none on any line this PR touches. ### For code changes: - [x] Does the title of this PR start with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')? - [ ] Object storage: Have the integration tests been executed and the endpoint declared according to the connector-specific documentation? *N/A* - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? *N/A — no new dependencies* - [ ] If applicable, have you updated the `LICENSE`, `LICENSE-binary`, `NOTICE-binary` files? *N/A* ### AI Tooling If an AI tool was used: - [x] The PR includes the phrase "Contains content generated by <tool>" where <tool> is the name of the AI tool used. - [x] My use of AI contributions follows the ASF legal policy https://www.apache.org/legal/generative-tooling.html -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
