joseluisll commented on code in PR #8800:
URL: https://github.com/apache/hadoop/pull/8800#discussion_r4238416978
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -719,9 +720,14 @@ public <S extends SchedulableEntity> OrderingPolicy<S>
getAppOrderingPolicy(
Map<String, String> config = new HashMap<String, String>();
String confPrefix = getQueuePrefix(queue) + ORDERING_POLICY + ".";
- for (Map.Entry<String, String> kv : this) {
- if (kv.getKey().startsWith(confPrefix)) {
- config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+ Properties props = getProps();
+ synchronized (props) {
+ for (Map.Entry<Object, Object> kv : props.entrySet()) {
Review Comment:
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](https://claude.ai/code)_
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -719,9 +720,14 @@ public <S extends SchedulableEntity> OrderingPolicy<S>
getAppOrderingPolicy(
Map<String, String> config = new HashMap<String, String>();
String confPrefix = getQueuePrefix(queue) + ORDERING_POLICY + ".";
- for (Map.Entry<String, String> kv : this) {
- if (kv.getKey().startsWith(confPrefix)) {
- config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+ Properties props = getProps();
+ synchronized (props) {
+ for (Map.Entry<Object, Object> kv : props.entrySet()) {
Review Comment:
`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](https://claude.ai/code)_
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -719,9 +720,14 @@ public <S extends SchedulableEntity> OrderingPolicy<S>
getAppOrderingPolicy(
Map<String, String> config = new HashMap<String, String>();
String confPrefix = getQueuePrefix(queue) + ORDERING_POLICY + ".";
- for (Map.Entry<String, String> kv : this) {
- if (kv.getKey().startsWith(confPrefix)) {
- config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+ Properties props = getProps();
Review Comment:
`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](https://claude.ai/code)_
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -719,9 +720,14 @@ public <S extends SchedulableEntity> OrderingPolicy<S>
getAppOrderingPolicy(
Map<String, String> config = new HashMap<String, String>();
String confPrefix = getQueuePrefix(queue) + ORDERING_POLICY + ".";
- for (Map.Entry<String, String> kv : this) {
- if (kv.getKey().startsWith(confPrefix)) {
- config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+ Properties props = getProps();
Review Comment:
The commit message explains why this doesn't iterate over `this`, but the
code doesn't, so someone could "simplify" it back. Something like:
```java
// Avoid Configuration.iterator(), which copies all properties on each call;
// same filter and lock as iterator().
```
---
_Generated by [Claude Code](https://claude.ai/code)_
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -719,9 +720,14 @@ public <S extends SchedulableEntity> OrderingPolicy<S>
getAppOrderingPolicy(
Map<String, String> config = new HashMap<String, String>();
String confPrefix = getQueuePrefix(queue) + ORDERING_POLICY + ".";
- for (Map.Entry<String, String> kv : this) {
- if (kv.getKey().startsWith(confPrefix)) {
- config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+ Properties props = getProps();
+ synchronized (props) {
+ for (Map.Entry<Object, Object> kv : props.entrySet()) {
+ if (kv.getKey() instanceof String && kv.getValue() instanceof String
Review Comment:
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](https://claude.ai/code)_
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -719,9 +720,14 @@ public <S extends SchedulableEntity> OrderingPolicy<S>
getAppOrderingPolicy(
Map<String, String> config = new HashMap<String, String>();
String confPrefix = getQueuePrefix(queue) + ORDERING_POLICY + ".";
- for (Map.Entry<String, String> kv : this) {
- if (kv.getKey().startsWith(confPrefix)) {
- config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+ Properties props = getProps();
+ synchronized (props) {
+ for (Map.Entry<Object, Object> kv : props.entrySet()) {
+ 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());
Review Comment:
nit: optional, but this does each cast once:
```java
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](https://claude.ai/code)_
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -719,9 +720,14 @@ public <S extends SchedulableEntity> OrderingPolicy<S>
getAppOrderingPolicy(
Map<String, String> config = new HashMap<String, String>();
String confPrefix = getQueuePrefix(queue) + ORDERING_POLICY + ".";
- for (Map.Entry<String, String> kv : this) {
- if (kv.getKey().startsWith(confPrefix)) {
- config.put(kv.getKey().substring(confPrefix.length()), kv.getValue());
+ Properties props = getProps();
+ synchronized (props) {
Review Comment:
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](https://claude.ai/code)_
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java:
##########
@@ -72,6 +72,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Properties;
Review Comment:
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](https://claude.ai/code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
