HADOOP-19982. Re-enable TestDFSClientRetries and TestBalancerWithHANameNodes in GHA - #8725
Open
joseluisll wants to merge 3 commits into
Open
joseluisll wants to merge 3 commits into
joseluisll wants to merge 3 commits into
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…amer cannot strand a writer A Throwable that escapes the handler at the bottom of run()'s loop killed the streamer thread before it reached closeInternal(). That left streamerClosed false and nothing to call dataQueue.notifyAll(), so a writer parked in waitForAckedSeqno waited on a thread that no longer existed until the datanode write timeout expired -- 495s with the defaults, since the guard is dfs.datanode.socket.write.timeout (8 min) plus 5s per node, and the pipeline is not yet up when close() starts waiting, so the node count falls back to 3. The escape seen in practice is the assertion two lines below lastException.set: lastException.set(e); assert !(e instanceof NullPointerException); With assertions enabled -- Surefire's default, so every Hadoop test JVM -- an NPE in the streamer raises AssertionError inside the catch block. The exception had already been recorded in lastException one line earlier, but waitForAckedSeqno only rethrows it via checkClosed() when streamerClosed is set, which the AssertionError skipped. The error was therefore both recorded and undeliverable. Captured while wedged, main holds both monitors and waits on a producer that is gone; the whole 112-thread dump contains exactly one DataStreamer frame, main's own: "main" #1 ... in Object.wait() java.lang.Thread.State: TIMED_WAITING (on object monitor) at org.apache.hadoop.hdfs.DataStreamer.waitForAckedSeqno(DataStreamer.java:966) - locked <0x000000070cad7e28> (a java.util.LinkedList) at org.apache.hadoop.hdfs.DFSOutputStream.flushInternal(DFSOutputStream.java:797) at org.apache.hadoop.hdfs.DFSOutputStream.closeImpl(DFSOutputStream.java:914) - locked <0x000000070cc5c2b8> (a org.apache.hadoop.hdfs.DFSOutputStream) Three dumps ten seconds apart were identical: same frame, same monitors, 112 threads, and 1.66ms of CPU across 20s of wall clock -- the one-second poll ticking and nothing else. Move the loop into runLoop() and call closeInternal() from a finally block, so it runs however the thread ends. The loop body is unchanged. This fixes the class of failure rather than the one instance: any Throwable that kills the streamer -- AssertionError, OutOfMemoryError -- now still sets streamerClosed, wakes the waiters and lets checkClosed() deliver lastException, so close() fails with the real cause instead of hanging. The assertion is left in place; its intent, surfacing NPEs loudly, is preserved, and it now does so promptly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…meNodes in GHA
Both classes were excluded before the fixes that addressed their failure modes
landed, and .github/gha-tests/exclude-tests.txt has not been revised since.
TestBalancerWithHANameNodes: HDFS-17957 raised testBalancerWithHANameNodes and
testBalancerRequestSBNWithHA from 60s to 300s and dropped dead code that pinned
the default NameNode RPC port. This change also raises
testGetLiveDatanodeStorageReport, the one method still on the original 60s
budget, to 180s. That is a precautionary ceiling rather than a measured
requirement: the HA cluster bring-up dominates the method and 60s left no
margin for it alone, and 180s is the largest budget the observer tests in the
same class already use for comparable cluster work. The class has been clean
wherever it ran: 47.13s and 47.28s in two Yetus builds, green in GHA.
TestDFSClientRetries needed more than HDFS-17972. That change bounded the
DFSClientFaultInjector latch and restored the injector in a finally block,
which did stop a dying testLeaseRenewAndDFSOutputStreamDeadLock from hanging
the rest of the class -- a thread dump taken during a hang confirms the lease
renewer is healthy and sleeping in its normal loop. But the method itself still
wedged for its whole timeout, twice out of two Yetus builds and twice out of
seven local runs, for two reasons. The first is the streamer thread dying
without releasing the writer, fixed in the preceding commit. The second is this
test stubbing its own NameNode spy from a second thread:
Thread closeThread = new Thread(() ->
Mockito.doThrow(new SocketTimeoutException()).when(spyNN).renewLease(...));
closeThread.start();
out1.close();
while the lease renewer called renewLease() every 100ms and close() drove
addBlock() and complete() through the same spy. Mockito keeps doAnswer-style
stubbing state on the mock rather than per thread -- InvocationContainerImpl
holds doAnswerStyleStubbing and invocationForStubbing as instance fields, and
MockHandlerImpl.handle() opens with
if (invocationContainer.hasAnswersForStubbing()) {
... setMethodForStubbing(invocationMatcher); return null; }
so between when(spyNN) publishing the answer and the stubbing call consuming
it, an invocation from any thread takes that branch: it binds the
SocketTimeoutException to its own method and returns null without running the
real one. Three outcomes were observed, all on calls the streamer or the
renewer made:
java.lang.NullPointerException: Cannot invoke "LocatedBlock.getBlock()"
because "lb" is null
at DataStreamer.setupPipelineForCreate(DataStreamer.java:1833)
java.lang.AssertionError
at org.mockito.internal.stubbing.InvocationContainerImpl
.setMethodForStubbing(InvocationContainerImpl.java:123)
at NameNodeRpcServer.addBlock(NameNodeRpcServer.java:933)
and renewLease() consuming its own stubbing, after which the renewer never
threw and the test passed without exercising anything.
Stub renewLease on the test thread, before the DFSClient exists, and drop the
thread. This does not weaken the test: renewLease() is a no-op while no file is
being written, so the renewer still fails on its first renewal after out1 is
created, still parks in the fault injector holding the LeaseRenewer monitor,
and close() still has to take that monitor to end the file lease. The thread
never synchronised with close() anyway, so the ordering it appeared to create
was never guaranteed.
Both classes run in the "hdfs - other" matrix entry
(-pl :hadoop-hdfs -DexcludedGroups=slow); neither carries @tag("slow"), so a
single matrix entry exercises both removals. Neither class is excluded anywhere
else in the repository.
Per .github/gha-tests/README.md, these removals need five consecutive
successful GHA runs before they are considered stable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
joseluisll
force-pushed
the
HADOOP-19982-reenable-gha-tests
branch
from
September 10, 2026 08:10
8889cfa to
829aff8
Compare
…write TestDFSOutputStream#testWriterIsReleasedWhenStreamerDies pins the invariant the preceding DataStreamer change restores: a streamer thread that ends without running closeInternal() must not leave a writer blocked in close(). The failure is driven the same way it occurred in practice, deterministically rather than by a race: addBlock() is stubbed to return null, which takes the streamer into a NullPointerException in setupPipelineForCreate(). With assertions enabled, Surefire's default, the assertion in run()'s own handler then raises AssertionError from inside the error handling, so the thread dies before setting streamerClosed or notifying dataQueue. No product hook is needed, and no assumption is made about which of the two paths ends the thread: the test only requires that the writer is released and told what happened. close() runs on its own thread with a bounded join, rather than relying on @timeout alone, so a regression reports close() was still blocked 30s after the streamer died. A streamer that ends without closeInternal() never sets streamerClosed and never notifies dataQueue, stranding the writer in waitForAckedSeqno until the datanode write timeout expires. instead of an unexplained timeout -- which is what made the original bug expensive to find, since the wait is bounded only by the 495s datanode write timeout and every test budget is shorter than that. The lease cleanup is skipped while the writer is still blocked. It holds the DFSOutputStream monitor there, and abort() takes the same monitor, so cleaning up unconditionally would block as well and hang the fork rather than reporting the assertion. Verified in both directions on the same tree: with the fix the test passes in 6.3s and 7.7s; with the fix reverted it fails in 39s on the assertion above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
Removes
TestDFSClientRetriesandTestBalancerWithHANameNodesfrom.github/gha-tests/exclude-tests.txt. Both were excluded before the fixes fortheir failure modes landed:
DFSClientFaultInjectorlatch wait and restores the previous injector in a
finally, so a dyingtestLeaseRenewAndDFSOutputStreamDeadLockno longer hangs the rest of the class.This PR
also raises:
testGetLiveDatanodeStorageReport, the one method still on theoriginal 60s budget, to 180s.
TestDFSClientRetries#testLeaseRenewAndDFSOutputStreamDeadLockfrom 120s to 300s.How was this patch tested?
Full GHA build on the branch: https://github.com/joseluisll/hadoop/actions/runs/34113842050 — all jobs green.
Both classes run in the
hdfs - othermatrix entry, which passed.Per
.github/gha-tests/README.md, removals need 5 consecutive successful GHAruns before they are considered stable; this is 1 of 5.
For code changes:
AI Tooling
Contains content generated by Claude Code.