YARN-12012. Collect Capacity Scheduler application ordering policy parameters without copying the configuration by brumi1024 · Pull Request #8800 · apache/hadoop

brumi1024 · 2026-10-07T12:06:19Z

Description of PR

CapacitySchedulerConfiguration.getAppOrderingPolicy iterated the whole configuration once per leaf queue through Configuration.iterator(), which copies every property into a new map, so building Q leaf queues copied the N properties Q times. The parameters are now collected from the properties directly, under their lock, with the filter that Configuration.iterator() applies, so each policy receives the same map.

This is independent of the configuration resolver series that follows, and makes it possible to benchmark that series against trunk with this change applied.

For more details see: YARN-12010

How was this patch tested?

TestLeafQueue#testFairConfiguration

For code changes:

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? Note: Automated CI
testing doesn't cover all cases so manual testing with cloud storage is still
required.
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

AI Tooling

If an AI tool was used:

The PR includes the phrase "Contains content generated by "
where is the name of the AI tool used.
My use of AI contributions follows the ASF legal policy
https://www.apache.org/legal/generative-tooling.html

Contains content generated by Claude Code

…rameters without copying the configuration CapacitySchedulerConfiguration.getAppOrderingPolicy iterated the whole configuration once per leaf queue through Configuration.iterator(), which copies every property into a new map, so building Q leaf queues copied the N properties Q times. The parameters are now collected from the properties directly, under their lock, with the filter that Configuration.iterator() applies, so each policy receives the same map. This is independent of the configuration resolver series that follows, and makes it possible to benchmark that series against trunk with this change applied.

joseluisll

Thanks, the change looks correct to me. I compared it with Configuration.iterator(): same String-only filter, same lock on props, same raw values with no variable expansion. So OrderingPolicy.configure() gets the same map as before, and configure() still runs outside the lock.

Yetus is green apart from test4tests (-1). That covers compile on JDK 17/21, checkstyle (the patch removes one warning), spotbugs, javadoc, and the resourcemanager unit tests (3405 run, 0 failures). The one flake, TestCapacitySchedulerSchedulingRequestUpdate#testNodePartitionPendingResourceUpdate, fails in CSMaxRunningAppsEnforcer.untrackRunnableApp, which looks unrelated to this change.

A few questions and nits are inline. The main ones are about how big the gain is, and whether the per-queue scan could be removed altogether.

Generated by Claude Code

joseluisll · 2026-10-10T16:52:44Z

-         config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+    Properties props = getProps();
+    synchronized (props) {
+      for (Map.Entry<Object, Object> kv : props.entrySet()) {


This removes the per-queue copy, but each leaf queue still scans every property, so it's still Q×N entry visits and only the constant factor changes. Could you add your before/after numbers (Q, N, time or allocations) to the JIRA? Is the remaining O(Q·N) cost something the resolver series will remove?

Generated by Claude Code

Yes, this only removes the per-queue copy; each leaf queue still scans all properties, so it is still O(Q·N) with a smaller constant.
I'll add the numbers to the JIRA. With 1000 queues (medians of 3 interleaved forks), trunk vs. trunk with this patch: load 167 → 124 ms, refresh 227 → 162 ms.
And yes, the resolver series will remove the remaining scan: from YARN-12016 on, leaf queues get their ordering policy parameters from the resolver, which reads them through a prefix index built once per configuration (ConfigSnapshot). I just didn't want to create the PR yet, as it would contain this commit as well.

joseluisll · 2026-10-10T16:52:44Z

-         config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+    Properties props = getProps();
+    synchronized (props) {
+      for (Map.Entry<Object, Object> kv : props.entrySet()) {


getConfiguredNodeLabels(QueuePath) has the same scan-per-queue pattern. It was solved by building ConfiguredNodeLabels once per configuration, and QueueNodeLabelsSettings only falls back to the per-queue scan when that's missing. Could the *.ordering-policy.* parameters be collected for all queues in one pass at reinit? That would make it O(N).

Generated by Claude Code

Agreed that one pass is the right shape, and that is what YARN-12016 does: the configuration is indexed by key prefix once, and every queue's ordering policy parameters come from that index.
I'd rather not add a second mechanism like ConfiguredNodeLabels here only for it to be replaced later on. This patch is meant to stay minimal so it also works as the benchmark baseline for the series.

joseluisll · 2026-10-10T16:52:44Z

-    for (Map.Entry<String, String> kv : this) {
-      if (kv.getKey().startsWith(confPrefix)) {
-         config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+    Properties props = getProps();


ConfigurationProperties already indexes properties by prefix (it's used in getConfiguredNodeLabelsByQueue()). My guess is you avoided it because set() doesn't invalidate that cache, and TestLeafQueue#testFairConfiguration calls set() and then getAppOrderingPolicy again. If that's the reason, it's worth a line in a code comment here.

Generated by Claude Code

Yep, that is the reason: set() does not invalidate the cached ConfigurationProperties, and TestLeafQueue#testFairConfiguration depends on set() followed by getAppOrderingPolicy. It also casts the properties to Map<String, String> without filtering non-String entries. Added code comments.

joseluisll · 2026-10-10T16:52:44Z

-    for (Map.Entry<String, String> kv : this) {
-      if (kv.getKey().startsWith(confPrefix)) {
-         config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+    Properties props = getProps();


The commit message explains why this doesn't iterate over this, but the code doesn't, so someone could "simplify" it back. Something like:

// Avoid Configuration.iterator(), which copies all properties on each call; // same filter and lock as iterator().

Generated by Claude Code

joseluisll · 2026-10-10T16:52:44Z

+    Properties props = getProps();
+    synchronized (props) {
+      for (Map.Entry<Object, Object> kv : props.entrySet()) {
+        if (kv.getKey() instanceof String && kv.getValue() instanceof String


Yetus voted -1 on test4tests. Since this is meant not to change behaviour, it's fine either to say in the PR that TestLeafQueue#testFairConfiguration already covers this path, or to add a small test. The test could put a non-String value into the configuration's Properties and check that it's skipped, which pins down the "same filter as iterator()" promise.

Generated by Claude Code

This is meant not to change behaviour, and the path is already covered by TestLeafQueue#testFairConfiguration, which sets ordering policy parameters with set() and reads them back through getAppOrderingPolicy. It passes along with the rest of the resourcemanager tests.
I'd skip the non-String test: getProps() is protected and the public Configuration API never stores non-String values, so the filter is only there to match iterator().

joseluisll · 2026-10-10T16:52:44Z

-      if (kv.getKey().startsWith(confPrefix)) {
-         config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+    Properties props = getProps();
+    synchronized (props) {


nit (design): This copies Configuration.iterator()'s internals (the non-String filter and the lock on props) into a YARN class, so the two could drift apart. A helper in Configuration that returns raw properties for a prefix would keep this logic in one place. The existing getPropsWithPrefix can't be swapped in directly, because it goes through get() (which expands variables) and doesn't take the lock. Fine to leave as is if you'd rather not touch hadoop-common.

Generated by Claude Code

Agreed that a raw prefix accessor in Configuration would be cleaner, but I'd rather keep this patch YARN-only. Later in the series the queue build path reads from ConfigSnapshot instead, so this loop stops being the hot path.

joseluisll · 2026-10-10T16:52:44Z

+        if (kv.getKey() instanceof String && kv.getValue() instanceof String
+            && ((String) kv.getKey()).startsWith(confPrefix)) {
+          config.put(((String) kv.getKey()).substring(confPrefix.length()),
+              (String) kv.getValue());


nit: optional, but this does each cast once:

for (Map.Entry<Object, Object> kv : props.entrySet()) { if (kv.getKey() instanceof String && kv.getValue() instanceof String) { String key = (String) kv.getKey(); if (key.startsWith(confPrefix)) { config.put(key.substring(confPrefix.length()), (String) kv.getValue()); } } }

Generated by Claude Code

joseluisll · 2026-10-10T16:52:44Z

 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Properties;


nit: java.util.Properties should go after java.util.Map.Entry. Checkstyle didn't flag it, so this is cosmetic only.

Generated by Claude Code

… parameters.

hadoop-yetus · 2026-10-11T12:46:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	20m 1s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	47m 40s		trunk passed
+1 💚	compile	1m 24s		trunk passed with JDK Ubuntu-21.0.12.1+1-1-24.04.4-Ubuntu
+1 💚	compile	1m 26s		trunk passed with JDK Ubuntu-17.0.20.1+1-1-24.04-Ubuntu
+1 💚	checkstyle	1m 24s		trunk passed
+1 💚	mvnsite	1m 32s		trunk passed
+1 💚	javadoc	1m 14s		trunk passed with JDK Ubuntu-21.0.12.1+1-1-24.04.4-Ubuntu
+1 💚	javadoc	1m 11s		trunk passed with JDK Ubuntu-17.0.20.1+1-1-24.04-Ubuntu
+1 💚	spotbugs	2m 42s		trunk passed
+1 💚	shadedclient	36m 9s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 1s		the patch passed
+1 💚	compile	0m 55s		the patch passed with JDK Ubuntu-21.0.12.1+1-1-24.04.4-Ubuntu
+1 💚	javac	0m 55s		the patch passed
+1 💚	compile	0m 59s		the patch passed with JDK Ubuntu-17.0.20.1+1-1-24.04-Ubuntu
+1 💚	javac	0m 59s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 52s		hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager: The patch generated 0 new + 30 unchanged - 1 fixed = 30 total (was 31)
+1 💚	mvnsite	1m 3s		the patch passed
+1 💚	javadoc	0m 45s		the patch passed with JDK Ubuntu-21.0.12.1+1-1-24.04.4-Ubuntu
+1 💚	javadoc	0m 45s		the patch passed with JDK Ubuntu-17.0.20.1+1-1-24.04-Ubuntu
+1 💚	spotbugs	2m 19s		the patch passed
+1 💚	shadedclient	35m 16s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	114m 42s		hadoop-yarn-server-resourcemanager in the patch passed.
+1 💚	asflicense	0m 36s		The patch does not generate ASF License warnings.
		274m 0s

Subsystem	Report/Notes
Docker	ClientAPI=1.56 ServerAPI=1.56 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-8800/3/artifact/out/Dockerfile
GITHUB PR	#8800
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux 9313804caf99 5.15.0-185-generic #195-Ubuntu SMP Fri Jun 19 17:11:50 UTC 2026 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `a30bbd9`
Default Java	Ubuntu-17.0.20.1+1-1-24.04-Ubuntu
Multi-JDK versions	/usr/lib/jvm/java-21-openjdk-amd64:Ubuntu-21.0.12.1+1-1-24.04.4-Ubuntu /usr/lib/jvm/java-17-openjdk-amd64:Ubuntu-17.0.20.1+1-1-24.04-Ubuntu
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-8800/3/testReport/
Max. process+thread count	889 (vs. ulimit of 10000)
modules	C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-8800/3/console
versions	git=2.43.0 maven=3.9.15 spotbugs=4.9.7
Powered by	Apache Yetus 0.14.1 https://yetus.apache.org

This message was automatically generated.

joseluisll

Thanks @brumi1024, all points are addressed, and I agree with keeping this minimal as the baseline for YARN-12016's one-pass index.
Please name TestLeafQueue#testFairConfiguration under "How was this patch tested?" to answer test4tests. +1 (non-binding).

brumi1024 · 2026-10-12T08:00:28Z

Thanks @joseluisll. @slfan1989 can you please approve if you have the chance?

slfan1989

LGTM.

Thanks for addressing the review comments and providing the benchmark results. The existing TestLeafQueue#testFairConfiguration coverage is sufficient for this behavior-preserving optimization. All build and test checks passed; the remaining Yetus -1 is only the test4tests warning, which is explained in the PR. +1.

brumi1024 · 2026-10-12T08:49:23Z

Thanks @joseluisll and @slfan1989 for the reviews! Merging to trunk.

github-actions Bot added YARN trunk labels Oct 7, 2026

brumi1024 force-pushed the YARN-12012 branch from 813ae53 to d40f0c4 Compare October 7, 2026 12:26

joseluisll reviewed Oct 10, 2026

View reviewed changes

slfan1989 assigned brumi1024 Oct 11, 2026

YARN-12012. Address review comments on collecting the ordering policy…

a30bbd9

… parameters.

brumi1024 requested a review from joseluisll October 11, 2026 11:49

joseluisll approved these changes Oct 11, 2026

View reviewed changes

slfan1989 approved these changes Oct 12, 2026

View reviewed changes

brumi1024 merged commit 86e6c20 into apache:trunk Oct 12, 2026
3 of 5 checks passed

brumi1024 deleted the YARN-12012 branch October 12, 2026 08:49

Conversation

brumi1024 commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

How was this patch tested?

For code changes:

AI Tooling

Uh oh!

joseluisll 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!

hadoop-yetus commented Oct 11, 2026

Uh oh!

joseluisll left a comment

Choose a reason for hiding this comment

Uh oh!

brumi1024 commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slfan1989 left a comment

Choose a reason for hiding this comment

Uh oh!

brumi1024 commented Oct 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brumi1024 commented Oct 7, 2026 •

edited

Loading

brumi1024 commented Oct 12, 2026 •

edited

Loading