HIVE-27126: queue level resource stats for YARN RM. by architjainjain · Pull Request #6501 · apache/hive

architjainjain · 2026-05-20T12:48:01Z

What changes were proposed in this pull request?

Display Yarn resources availability in real time along with Tez job progress

Why are the changes needed?

Right we don't know if the queue used in Hive query execution is having enough resources or not. We are already displaying tez job details like number of task per each vertex and how are they progressing.

If the resources available are not good enough to execute the query in parallel or query that use to take shorter time is taking time, then we can use this new detail to understand that queue is busy and resources are not enough

Does this PR introduce any user-facing change?

yes, it shows queue level metrics.

How was this patch tested?

the patch was tested by creating yarn cluster and running queries, with different queues.

after disabling the queue metrics:
set hive.tez.queue.metrics.refresh.interval=0s;

sonarqubecloud · 2026-06-17T16:41:10Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
1 Security Hotspot
0.0% Coverage on New Code
0.3% Duplication on New Code

See analysis details on SonarQube Cloud

abstractdog

thanks @architjainjain so far, dropped some comments, but I wasn't able to fully read it through, I'll get back after, in the meantime I can do some testing too hopefully

abstractdog · 2026-06-22T09:24:06Z

+ * behaviour added as part of HIVE-27126.
+ *
+ * We capture stdout via a ByteArrayOutputStream and inspect the rendered output.
+ */public class TestInPlaceUpdate {


line break before public

abstractdog · 2026-06-22T09:30:35Z

+      new String(new char[InPlaceUpdate.MIN_TERMINAL_WIDTH]).replace("\0", "-");
+
+  /** Counts non-overlapping occurrences of {@code needle} in {@code haystack}. */
+  private static int countOccurrences(String haystack, String needle) {


what about reusing commons-lang3 StringUtils.countMatches

abstractdog · 2026-06-22T09:31:26Z

+ * Unit tests for InPlaceUpdate — focusing on the queue-metrics separator
+ * behaviour added as part of HIVE-27126.


TestInPlaceUpdate sounds like a generic unit test class for InPlaceUpdate, so this comment is supposed to get outdated over time

abstractdog · 2026-06-22T09:32:23Z

+  /** Minimal ProgressMonitor stub — returns empty headers/rows/footer. */
+  private static ProgressMonitor makeMonitor(String queueMetrics) {
+    return new ProgressMonitor() {
+      @Override public List<String> headers() {


public in new line, pls take care of the same below too

abstractdog · 2026-06-22T09:35:00Z

+
+
+
+
+


remove extra lines

abstractdog · 2026-06-22T12:30:11Z

+      for (int i = 0; i < 5; i++) {
+        collectors[i] = new YarnQueueMetricsCollector(
+            mockYarnClient, "default", refreshIntervalMs, "jitter-test-query-" + i);
+        assertNotNull("Collector " + i + " should be created successfully", collectors[i]);
+      }
+      // If we get here, all 5 collectors were created with their own jittered delays
+      // without conflict or exception - thundering herd fix is in place


considering mock yarn clients, creating concurrent collectors for jitter testing doesn't seem that valuable to me...I don't think they will ever throw an exception

abstractdog · 2026-06-22T12:32:54Z

+        mockYarnClient, "default", 30, "circuit-breaker-recovery-query");
+
+    try {
+      // Wait for recovery - snapshot should eventually be populated


need extra assertion about the failure first to make 100% sure we actually hit circuit breaker logic

abstractdog · 2026-06-22T12:34:18Z

+  public void testCircuitBreakerDoesNotAffectSuccessfulCollection() throws Exception {
+    // Normal operation - no failures, circuit breaker should never activate


this is just a simple happy testing, I guess it's covered by testSuccessfulMetricsCollection

@abstractdog Thanks for catching this! I've added JavaDoc to clarify that this test is part of the circuit breaker test suite. While it looks similar to testSuccessfulMetricsCollection(), it specifically validates that the circuit breaker doesn't interfere with normal operations. The circuit breaker suite needs three scenarios: failure, recovery, and happy path. This test covers the happy path behavior for circuit breaker specifically, not general metrics collection. I've added cross-references to the related tests for clarity.

abstractdog · 2026-06-22T12:38:56Z

+    when(mockQueueStats.getAllocatedMemoryMB()).thenReturn(1024L);
+    when(mockQueueStats.getAvailableMemoryMB()).thenReturn(1024L);
+    when(mockQueueStats.getAllocatedVCores()).thenReturn(4L);
+    when(mockQueueStats.getAvailableVCores()).thenReturn(4L);
+    when(mockQueueStats.getNumAppsRunning()).thenReturn(1L);
+    when(mockQueueStats.getPendingContainers()).thenReturn(0L);
+    when(mockQueueInfo.getQueueStatistics()).thenReturn(mockQueueStats);
+    when(mockQueueInfo.getCapacity()).thenReturn(0.5f);
+    when(mockYarnClient.getQueueInfo("default")).thenReturn(mockQueueInfo);


I feel like defining all these variables adds a lot of boilerplate to the unit tests, whereas in this case we're only interested in collection timestamp, could a separate method be created just for adding happy value, and it can be overridden per test, like:

when(mockQueueStats.getAllocatedMemoryMB()).thenReturn(1024L); when(mockQueueStats.getAvailableMemoryMB()).thenReturn(1024L); when(mockQueueStats.getAllocatedVCores()).thenReturn(4L); when(mockQueueStats.getAvailableVCores()).thenReturn(4L); when(mockQueueStats.getNumAppsRunning()).thenReturn(1L); when(mockQueueStats.getPendingContainers()).thenReturn(0L); when(mockQueueInfo.getQueueStatistics()).thenReturn(mockQueueStats); when(mockQueueInfo.getCapacity()).thenReturn(0.5f); when(mockYarnClient.getQueueInfo("default")).thenReturn(mockQueueInfo);

method javadoc can also tell that these are only default happy values for the unit tests

abstractdog · 2026-06-22T12:45:05Z

+      // HIVE-27126: Thrift regeneration omitted setStartTimeIsSet(true) from the constructor.
+      // Explicitly call setStartTime() to set the isset flag required for Thrift validation.
+      tProgressUpdateResp.setStartTime(progressUpdate.startTimeMillis);
+      if (progressUpdate.queueMetrics() != null && !progressUpdate.queueMetrics().isEmpty()) {
+        tProgressUpdateResp.setQueueMetrics(progressUpdate.queueMetrics());
+      }
+      resp.setProgressUpdateResponse(tProgressUpdateResp);


why is this change needed?

This works around a Thrift code generation bug. When we regenerated Thrift code after adding the queueMetrics field, the generated constructor for TProgressUpdateResp accepts startTimeMillis but fails to set the isset flag. Without this flag, Thrift serialization treats the field as unset, causing clients to receive incomplete progress updates. The explicit setStartTime() call ensures both the value AND the isset flag are properly set, maintaining backward compatibility with Thrift clients.

without this test the generated file is not having the starttime isset flag updated.

https://github.com/apache/hive/pull/6501/changes#diff-d71ddeafeea57e0fbf6a9dfda436eab06b69a39ac46c815cc1b15bfffe5f35e0L155

architjainjain · 2026-06-23T05:51:55Z

    this.status = status;
    this.footerSummary = footerSummary;
    this.startTime = startTime;
-    setStartTimeIsSet(true);


@abstractdog this is getting removed. so we added manually after the constructor call.

asf-ci-hive added tests pending tests failed and removed tests pending labels May 20, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from ea3b1b3 to 5f4c748 Compare June 1, 2026 09:52

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Jun 1, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch 4 times, most recently from 9e97b27 to e2d3173 Compare June 1, 2026 12:11

asf-ci-hive added tests pending tests passed and removed tests failed tests pending labels Jun 1, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from e2d3173 to fa844c0 Compare June 2, 2026 06:23

asf-ci-hive added tests pending and removed tests passed labels Jun 2, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from fa844c0 to dac58c2 Compare June 2, 2026 08:08

asf-ci-hive added tests passed tests pending tests unstable and removed tests pending tests passed labels Jun 2, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from dac58c2 to f4301f1 Compare June 8, 2026 13:32

asf-ci-hive added tests pending and removed tests unstable labels Jun 8, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from f4301f1 to ada32ce Compare June 8, 2026 15:45

asf-ci-hive added tests pending tests unstable and removed tests pending labels Jun 9, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from 8720321 to 827ee45 Compare June 10, 2026 08:35

asf-ci-hive added tests pending tests passed and removed tests unstable tests pending labels Jun 10, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from 827ee45 to 120c749 Compare June 10, 2026 11:33

architjainjain marked this pull request as ready for review June 10, 2026 11:44

asf-ci-hive added tests pending tests unstable and removed tests passed tests pending labels Jun 10, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from 120c749 to 34febff Compare June 15, 2026 07:24

asf-ci-hive added tests pending tests passed and removed tests unstable tests pending labels Jun 15, 2026

architjainjain force-pushed the HIVE-27126-yarn-RM branch from 34febff to 47b3abe Compare June 17, 2026 15:17

asf-ci-hive added tests pending and removed tests passed labels Jun 17, 2026

asf-ci-hive added tests failed and removed tests pending labels Jun 18, 2026

abstractdog requested changes Jun 22, 2026

View reviewed changes

architjainjain commented Jun 23, 2026

View reviewed changes

architjainjain force-pushed the HIVE-27126-yarn-RM branch from 47b3abe to e7a0726 Compare June 23, 2026 07:34

asf-ci-hive removed the tests failed label Jun 23, 2026

HIVE-27126: queue level resource stats for YARN RM.

1785832

		* Unit tests for InPlaceUpdate — focusing on the queue-metrics separator
		* behaviour added as part of HIVE-27126.

		public void testCircuitBreakerDoesNotAffectSuccessfulCollection() throws Exception {
		// Normal operation - no failures, circuit breaker should never activate

Conversation

architjainjain commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

sonarqubecloud Bot commented Jun 17, 2026

Quality Gate passed

Uh oh!

abstractdog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

architjainjain Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

architjainjain commented May 20, 2026 •

edited

Loading

architjainjain Jun 23, 2026 •

edited

Loading