Skip to content

HIVE-29842: fixing flaky TestYarnQueueMetricsCollector tests - #6729

Open
konstantinb wants to merge 2 commits into
apache:masterfrom
konstantinb:HIVE-29842
Open

HIVE-29842: fixing flaky TestYarnQueueMetricsCollector tests#6729
konstantinb wants to merge 2 commits into
apache:masterfrom
konstantinb:HIVE-29842

Conversation

@konstantinb

@konstantinb konstantinb commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

HIVE-29842: Test-only changes to TestYarnQueueMetricsCollector (no production code touched):

  • Await the first refresh poll (new awaitFirstPoll() helper: waitForInvocationCount + count assert, the same idiom the class already uses) in the three tests whose stubs are consumed only by the async refresh thread, before asserting and shutting down.
  • Remove four redundant strict stubs (and the throws Exception clauses only those stubs required): they either duplicate the lenient @Before defaults verbatim or set values the test's assertion never reads.
  • Document the invariant in the setupHappyPathMocks() javadoc: a strict per-test stub consumed only by the refresh thread must be awaited before shutdown.

Why are the changes needed?

Four tests stub interactions whose only consumer is the async refresh task (scheduled with scheduleWithFixedDelay(task, 0, ...)), then assert and call shutdown() without awaiting the poll. When shutdown() wins that race, the strict stub is never consumed and MockitoJUnitRunner fails the whole class with UnnecessaryStubbingException — this hit the precommit of an unrelated PR (run 6 of PR 6676; see HIVE-29842 for the link).

The tests usually pass only by accident: the collector's INFO logging during scheduling stalls the main thread long enough for the poll thread to sneak its call in. That makes the flake deterministic to reproduce on master:

mvn test -pl ql -Dtest=TestYarnQueueMetricsCollector -Dhive.log.level=OFF

(suppressing logging removes the log-I/O stall; the class then fails consistently pre-patch).

The awaits also make the assertions meaningful: previously assertNull(getLatestSnapshot()) passed whether the stubbed condition was exercised or the poll simply had not run yet.

Does this PR introduce any user-facing change?

No. Test-only.

How was this patch tested?

  • The repro command above: consistent failure pre-patch, repeated passes post-patch.
  • Full class with default logging: 23/23 green.
  • Checkstyle: no new violations on changed lines.

@sonarqubecloud

Copy link
Copy Markdown

@konstantinb

konstantinb commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

@abstractdog @architjainjain could you take a look when you have a chance? This fixes the flaky UnnecessaryStubbingException failures in TestYarnQueueMetricsCollector, introduced with the HIVE-27126 test suite (#6501), that have started hitting unrelated precommit runs — e.g. PR 6676 run 6 and run 7. Each run flagged a different test in the class (testMetricsCollectionWithNullQueueInfo, then testExceptionDuringCollection) — whichever strict stub loses the race with shutdown() gets reported; all of them are covered here. The PR description has a one-command deterministic repro; all checks on this PR are green.

Since this is a flaky-test fix: could a committer trigger a hive-flaky-check run for this branch as supporting evidence? I can view the job but don't have build permission. Parameters: GITHUB_USER=konstantinb, GITHUB_REPO=hive, BRANCH=HIVE-29842, OPTS=-pl ql -Dtest=TestYarnQueueMetricsCollector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants