HIVE-26089: ported TestAsyncPbRpcProxy to junit5, front-loaded JVM warmup and un-ignored a test - #6733
HIVE-26089: ported TestAsyncPbRpcProxy to junit5, front-loaded JVM warmup and un-ignored a test#6733konstantinb wants to merge 3 commits into
Conversation
|
@abstractdog @Aggarwal-Raghav could you take a look? This supersedes the |
Aggarwal-Raghav
left a comment
There was a problem hiding this comment.
Thanks @konstantinb for PR. I'm not a committer to this project so I can't trigger the flaky CI but I have given minor comments. Junit5 changes looks good.
NIT: It might be better to explicitly retain the timeout unit in milliseconds. As Hive progressively migrates to JUnit 5 (or eventually JUnit 6), keeping the exact same values?
| static void warmUp() { | ||
| mock(Message.class); | ||
| mock(LlapProtocolClientProxy.ExecuteRequestCallback.class); | ||
| org.slf4j.LoggerFactory.getLogger(TestAsyncPbRpcProxy.class).info("warm-up"); |
There was a problem hiding this comment.
Maybe use import instead of fully pacakge classification? Also, Logger can be defined as class attribute i.e.
private static final Logger LOG = LoggerFactory.getLogger(TestAsyncPbRpcProxy.class);
| @@ -32,13 +32,30 @@ | |||
| import org.apache.commons.lang3.mutable.MutableInt; | |||
| import org.apache.hadoop.hive.llap.LlapNodeId; | |||
There was a problem hiding this comment.
nit: Unused import
|



What changes were proposed in this pull request?
Test-only changes to
TestAsyncPbRpcProxy(no production code, no pom changes — JUnit 5 wasalready a llap-client test dependency):
@BeforeAll warmUp()with its own60-second
@Timeout: Mockito mock generation for the two mocked types, logging setup, andclassloading of the classes under test.
@Timeout(value = 5, threadMode = SEPARATE_THREAD)—SEPARATE_THREADis required for JUnit 4-equivalent preemption(Jupiter's default mode only interrupts, which would not stop a hard hang).
testSingleInvocationPerNode, disabled since 2022.This supersedes the approach of #6440 (
@IgnoreontestMultipleNodes) while restoring thecoverage removed by the 2022 direct-to-master disable
(cc6c02da9).
Why are the changes needed?
Both recorded failures of this class timed out during one-time JVM bootstrap, never in the
code under test: the original HIVE-26089 report died in classloading (
ZipFile.getEntry),and a recent precommit occurrence
(PR 6729, first run)
died inside the first
Mockito.mock()call while ByteBuddy generated the mock class. Withsurefire's
reuseForks=false, every test class starts a cold JVM, so this cost is paid perclass and lands on whichever timed test runs first. The 5s timeouts exist to catch deadlocks
in
RequestManager's lock/condition logic (the tests complete in ~0.5s) — they were nevermeant to time JVM startup.
The flake reproduces deterministically with no code changes by simulating a starved executor
with a CPU quota — a fresh JVM per run matches
reuseForks=false:At
--cpus=0.15the unmodified test fails every run (observed 4/4) with the exact CI stack —TestTimedOutExceptionfrozen inside Mockito/ByteBuddy bootstrap at the firstmock()call —and passes from
--cpus=0.35up (docker works the same as podman here).Splitting the budgets keeps both signals sharp: a 5s per-test timeout still catches a
deadlock, while an init overrun now fails as
warmUp() timed out— an environment problem,clearly distinguished from a test failure.
Does this PR introduce any user-facing change?
No. Test-only.
How was this patch tested?
patch both tests PASS at 0.15 and 0.10; at 0.05 (1/20 of a core) the init budget fails the
class with
warmUp() timed out after 60 secondsand zero test failures — the intendedbroken-executor signal.
the JUnit timeout thread (proving
SEPARATE_THREADpreemption); with a 1-millisecond initbudget, the class container fails naming
warmUp()before any test runs (proving theinit/test signal separation).
mvn test -pl llap-client -Dtest=TestAsyncPbRpcProxy: 2/2 green in ~0.5s (confirms surefirediscovers the Jupiter tests).