joseluisll commented on code in PR #8801:
URL: https://github.com/apache/hadoop/pull/8801#discussion_r4238405771
##########
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:
##########
@@ -1213,13 +1216,27 @@ public ConfigurationProperties
getConfigurationProperties() {
}
/**
- * Reinitializes the cached {@code ConfigurationProperties} object.
+ * Get the snapshot of this configuration. It is taken on first use and not
+ * refreshed by later writes into this object (for example template entries
+ * written for dynamic queues), until
+ * {@link #reinitializeConfigurationProperties()} is called.
+ * @return the cached configuration snapshot
+ */
+ public ConfigSnapshot getConfigSnapshot() {
+ if (configSnapshot == null) {
+ reinitializeConfigurationProperties();
+ }
+
+ return configSnapshot;
+ }
+
+ /**
+ * Reinitializes the cached {@code ConfigSnapshot} and the
+ * {@code ConfigurationProperties} view of it.
*/
- @SuppressWarnings({"unchecked", "rawtypes"})
public void reinitializeConfigurationProperties() {
- // Props are always Strings, therefore this cast is safe
- Map<String, String> props = (Map) getProps();
- configurationProperties = new ConfigurationProperties(props);
+ configSnapshot = ConfigSnapshot.of(this);
+ configurationProperties = new ConfigurationProperties(configSnapshot);
}
Review Comment:
YARN-12013 says the snapshot "replaces the manually invalidated
ConfigurationProperties cache". Here, `ConfigurationProperties` and
`reinitializeConfigurationProperties()` both remain, and `TestLeafQueue:1938`
still has to call it by hand. Is removing them planned for a later sub-task? If
so, could the JIRA or PR description say so, and could the method be marked
`@Deprecated`, or renamed to something like `refreshConfigSnapshot()` with the
old name delegating to it?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AutoCreatedQueueTemplate.java:
##########
@@ -106,12 +107,12 @@ public void
setTemplateEntriesForChild(CapacitySchedulerConfiguration conf,
return;
}
- ConfigurationProperties configurationProperties =
- conf.getConfigurationProperties();
-
- // Get all properties that are explicitly set
- Set<String> alreadySetProps = configurationProperties
- .getPropertiesWithPrefix(getQueuePrefix(childQueuePath)).keySet();
+ // Get all properties that are explicitly set. The snapshot is taken
+ // before any template entry is written, so entries written for this or
+ // other dynamic queues do not count as explicitly set.
+ Set<String> alreadySetProps = conf.getConfigSnapshot()
Review Comment:
Not a bug today, but a fragility. The "explicitly set" check in
`setTemplateEntriesForChild` is only correct because the snapshot is taken
before any template entry or dynamic-queue default (e.g.
`setUserLimitFactor(-1)` in `AbstractLeafQueue`) is written. Only the comment
in `CapacitySchedulerQueueContext#installConfiguration()` enforces that. Could
we make it explicit (for example a snapshot of the user-supplied config that is
never refreshed, or tracking the keys the template machinery wrote), or at
least add a test that fails if the snapshot is refreshed during queue setup?
##########
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:
##########
@@ -435,6 +436,7 @@ public class CapacitySchedulerConfiguration extends
ReservationSchedulerConfigur
private static final String LEGACY_QUEUE_MODE_ENABLED = PREFIX +
"legacy-queue-mode.enabled";
public static final boolean DEFAULT_LEGACY_QUEUE_MODE = true;
+ private ConfigSnapshot configSnapshot;
Review Comment:
Nit: `configSnapshot` and `configurationProperties` are now two non-volatile
fields assigned one after the other, and each getter initialises lazily on its
own. Since `ConfigurationProperties` is just a thin view,
`getConfigurationProperties()` could return `new
ConfigurationProperties(getConfigSnapshot())`, or the snapshot could own that
view. Then there is one cached field and no window where a reader sees a new
snapshot next to old properties.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/resolver/ConfigSnapshot.java:
##########
@@ -0,0 +1,304 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.resolver;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.conf.Configuration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An immutable copy of a {@link Configuration}, taken once, with a prefix
+ * index for per-queue and per-label lookups.
+ * <p>
+ * {@link #get(String)} and {@link #getPropertiesWithPrefix(String)} return
+ * the value {@code conf.get(key)} returned when the snapshot was taken, so
+ * deprecation handling and variable substitution are applied. The raw
+ * accessors return the unexpanded text stored in the configuration, which is
+ * what the prefix lookups of the Capacity Scheduler have always used.
+ * <p>
+ * The prefix index is a trie with one node per key part delimited by ".".
+ * A key is stored in the node of its last part, so a prefix query walks the
+ * prefix parts and collects the whole subtree below the last one. Prefixes
+ * therefore match whole key parts only: {@code root.a} matches
+ * {@code root.a.capacity} but not {@code root.ab.capacity}.
+ */
[email protected]
[email protected]
+public final class ConfigSnapshot {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ConfigSnapshot.class);
+
+ private static final String DELIMITER = "\\.";
+ private static final String DOT = ".";
+
+ /** Key to the value {@code conf.get(key)} returned. */
+ private final Map<String, String> values;
+ /**
+ * Key to the raw text, only for keys whose raw text differs from the
+ * value, which keeps the common case free of a second copy.
+ */
+ private final Map<String, String> rawValues;
+ /** Keys whose {@code conf.get(key)} threw, rethrown on access. */
+ private final Map<String, RuntimeException> failures;
+ private final Set<String> keys;
+ private final PrefixNode root = new PrefixNode();
+
+ private ConfigSnapshot(Map<String, String> values,
+ Map<String, String> rawValues, Map<String, RuntimeException> failures,
+ Set<String> keys) {
+ this.values = values;
+ this.rawValues = rawValues;
+ this.failures = failures;
+ this.keys = Collections.unmodifiableSet(keys);
+ for (String key : keys) {
+ index(key);
+ }
+ }
+
+ /**
+ * Takes a snapshot of every property of a configuration.
+ * @param conf the configuration to copy
+ * @return the snapshot
+ */
+ public static ConfigSnapshot of(Configuration conf) {
+ Map<String, String> raw = new HashMap<>();
+ // The iterator returns a copy of the raw properties, so the conf.get
+ // calls below, which may update deprecated keys, do not disturb it.
+ Iterator<Map.Entry<String, String>> it = conf.iterator();
+ while (it.hasNext()) {
+ Map.Entry<String, String> entry = it.next();
+ raw.put(entry.getKey(), entry.getValue());
+ }
+
+ Map<String, String> values = new HashMap<>(raw.size() * 4 / 3 + 1);
+ Map<String, String> rawValues = new HashMap<>();
+ Map<String, RuntimeException> failures = new HashMap<>();
+ for (Map.Entry<String, String> entry : raw.entrySet()) {
+ String key = entry.getKey();
+ String rawValue = entry.getValue();
+ // conf.get only differs from the raw text when the value references a
+ // variable or the key is deprecated; skipping the call keeps the
+ // snapshot cheap for the common case.
+ if (rawValue.indexOf('$') < 0 && !Configuration.isDeprecated(key)) {
+ values.put(key, rawValue);
+ continue;
+ }
+ try {
+ String value = conf.get(key);
+ if (value != null) {
+ values.put(key, value);
+ }
+ if (!rawValue.equals(value)) {
+ rawValues.put(key, rawValue);
+ }
+ } catch (RuntimeException e) {
+ failures.put(key, e);
+ rawValues.put(key, rawValue);
+ }
+ }
+ return new ConfigSnapshot(values, rawValues, failures, raw.keySet());
+ }
+
+ /**
+ * Takes a snapshot of plain properties, using every value as it is, without
+ * deprecation handling or variable substitution.
+ * @param properties the properties to copy
+ * @return the snapshot
+ */
+ public static ConfigSnapshot of(Map<String, String> properties) {
+ return new ConfigSnapshot(new HashMap<>(properties),
+ Collections.<String, String>emptyMap(),
+ Collections.<String, RuntimeException>emptyMap(),
+ new HashSet<>(properties.keySet()));
+ }
+
+ /**
+ * Returns the value of a property.
+ * @param key the full property key
+ * @return the value {@code conf.get(key)} returned, or {@code null} when
+ * the property is absent
+ * @throws RuntimeException the exception {@code conf.get(key)} threw, for
+ * example on a too deep variable substitution
+ */
+ public String get(String key) {
Review Comment:
Every production caller uses `getRaw*`. The variable-expanded accessors
(`get()`, `getPropertiesWithPrefix()`), `keys()` and the failure
capture/rethrow only run in tests. Meanwhile, every snapshot now calls
`conf.get()` on each value containing `$` and on each deprecated key across the
whole merged config. Would it be better to land the expanded side together with
its first consumer, so its semantics get reviewed against a real use? If it
stays, please put the load/refresh benchmark numbers mentioned in the JIRA in
the PR description.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/resolver/package-info.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Package
+ * org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.resolver
+ * contains the immutable configuration snapshot and the per-queue
+ * configuration resolution of the Capacity Scheduler.
Review Comment:
Nit: the package is called `resolver` and its javadoc mentions "per-queue
configuration resolution", but it only contains the snapshot so far. Fine if
YARN-12010's next sub-tasks fill it in. Otherwise `...capacity.conf` (which
already exists) might be a better home.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/resolver/ConfigSnapshot.java:
##########
@@ -0,0 +1,304 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.resolver;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.conf.Configuration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * An immutable copy of a {@link Configuration}, taken once, with a prefix
+ * index for per-queue and per-label lookups.
+ * <p>
+ * {@link #get(String)} and {@link #getPropertiesWithPrefix(String)} return
+ * the value {@code conf.get(key)} returned when the snapshot was taken, so
+ * deprecation handling and variable substitution are applied. The raw
+ * accessors return the unexpanded text stored in the configuration, which is
+ * what the prefix lookups of the Capacity Scheduler have always used.
+ * <p>
+ * The prefix index is a trie with one node per key part delimited by ".".
+ * A key is stored in the node of its last part, so a prefix query walks the
+ * prefix parts and collects the whole subtree below the last one. Prefixes
+ * therefore match whole key parts only: {@code root.a} matches
+ * {@code root.a.capacity} but not {@code root.ab.capacity}.
+ */
[email protected]
[email protected]
+public final class ConfigSnapshot {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(ConfigSnapshot.class);
+
+ private static final String DELIMITER = "\\.";
+ private static final String DOT = ".";
+
+ /** Key to the value {@code conf.get(key)} returned. */
+ private final Map<String, String> values;
+ /**
+ * Key to the raw text, only for keys whose raw text differs from the
+ * value, which keeps the common case free of a second copy.
+ */
+ private final Map<String, String> rawValues;
+ /** Keys whose {@code conf.get(key)} threw, rethrown on access. */
+ private final Map<String, RuntimeException> failures;
+ private final Set<String> keys;
+ private final PrefixNode root = new PrefixNode();
+
+ private ConfigSnapshot(Map<String, String> values,
+ Map<String, String> rawValues, Map<String, RuntimeException> failures,
+ Set<String> keys) {
+ this.values = values;
+ this.rawValues = rawValues;
+ this.failures = failures;
+ this.keys = Collections.unmodifiableSet(keys);
+ for (String key : keys) {
+ index(key);
+ }
+ }
+
+ /**
+ * Takes a snapshot of every property of a configuration.
+ * @param conf the configuration to copy
+ * @return the snapshot
+ */
+ public static ConfigSnapshot of(Configuration conf) {
+ Map<String, String> raw = new HashMap<>();
+ // The iterator returns a copy of the raw properties, so the conf.get
+ // calls below, which may update deprecated keys, do not disturb it.
+ Iterator<Map.Entry<String, String>> it = conf.iterator();
+ while (it.hasNext()) {
+ Map.Entry<String, String> entry = it.next();
+ raw.put(entry.getKey(), entry.getValue());
+ }
+
+ Map<String, String> values = new HashMap<>(raw.size() * 4 / 3 + 1);
+ Map<String, String> rawValues = new HashMap<>();
+ Map<String, RuntimeException> failures = new HashMap<>();
+ for (Map.Entry<String, String> entry : raw.entrySet()) {
+ String key = entry.getKey();
+ String rawValue = entry.getValue();
+ // conf.get only differs from the raw text when the value references a
+ // variable or the key is deprecated; skipping the call keeps the
+ // snapshot cheap for the common case.
+ if (rawValue.indexOf('$') < 0 && !Configuration.isDeprecated(key)) {
+ values.put(key, rawValue);
+ continue;
+ }
+ try {
+ String value = conf.get(key);
+ if (value != null) {
+ values.put(key, value);
+ }
+ if (!rawValue.equals(value)) {
+ rawValues.put(key, rawValue);
+ }
+ } catch (RuntimeException e) {
+ failures.put(key, e);
+ rawValues.put(key, rawValue);
+ }
+ }
+ return new ConfigSnapshot(values, rawValues, failures, raw.keySet());
+ }
+
+ /**
+ * Takes a snapshot of plain properties, using every value as it is, without
+ * deprecation handling or variable substitution.
+ * @param properties the properties to copy
+ * @return the snapshot
+ */
+ public static ConfigSnapshot of(Map<String, String> properties) {
+ return new ConfigSnapshot(new HashMap<>(properties),
+ Collections.<String, String>emptyMap(),
+ Collections.<String, RuntimeException>emptyMap(),
+ new HashSet<>(properties.keySet()));
+ }
+
+ /**
+ * Returns the value of a property.
+ * @param key the full property key
+ * @return the value {@code conf.get(key)} returned, or {@code null} when
+ * the property is absent
+ * @throws RuntimeException the exception {@code conf.get(key)} threw, for
+ * example on a too deep variable substitution
+ */
+ public String get(String key) {
+ RuntimeException failure = failures.get(key);
+ if (failure != null) {
+ throw failure;
Review Comment:
Nit: `get()` throws the same `RuntimeException` object that was caught when
the snapshot was built (line 122). Its stack trace points at snapshot
construction, not at the caller, and the same instance is thrown again on every
call, possibly from several threads. I'd suggest something like `throw new
IllegalStateException("Failed to resolve " + key, failure)`. Also, one bad key
now makes `getPropertiesWithPrefix()` throw for its whole prefix, while the raw
lookups never threw. Is that intended?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestConfigSnapshotTemplateWrites.java:
##########
@@ -0,0 +1,128 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity;
+
+import org.apache.hadoop.yarn.conf.YarnConfiguration;
+import org.apache.hadoop.yarn.server.resourcemanager.MockRM;
+import
org.apache.hadoop.yarn.server.resourcemanager.nodelabels.NullRMNodeLabelsManager;
+import
org.apache.hadoop.yarn.server.resourcemanager.nodelabels.RMNodeLabelsManager;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.ResourceScheduler;
+import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.resolver.ConfigSnapshot;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+
+/**
+ * Pins which readers see the values that v2 templates write into the shared
+ * queue context configuration while a dynamic queue is set up. The cached
+ * configuration snapshot is taken before any template entry is written, so
+ * readers going through it (user weights, the explicitly-set check of the
+ * templates) do not see them, while the ordinary getters do.
+ */
+public class TestConfigSnapshotTemplateWrites {
+ private static final QueuePath ROOT = new QueuePath("root");
+ private static final QueuePath A = new QueuePath("root.a");
+ private static final QueuePath B = new QueuePath("root.b");
+ private static final String AUTO = "root.a.auto1";
+ private static final String AUTO_PREFIX =
+ CapacitySchedulerConfiguration.PREFIX + AUTO + ".";
+ private static final String TEMPLATE =
+ AutoCreatedQueueTemplate.getAutoQueueTemplatePrefix(A);
+
+ private MockRM mockRM;
+ private CapacityScheduler cs;
+ private CapacitySchedulerConfiguration csConf;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ csConf = new CapacitySchedulerConfiguration();
+ csConf.setClass(YarnConfiguration.RM_SCHEDULER, CapacityScheduler.class,
+ ResourceScheduler.class);
+ csConf.setQueues(ROOT, new String[] {"a", "b"});
+ csConf.setNonLabeledQueueWeight(ROOT, 1f);
+ csConf.setNonLabeledQueueWeight(A, 1f);
+ csConf.setNonLabeledQueueWeight(B, 1f);
+ csConf.setAutoQueueCreationV2Enabled(A, true);
+ csConf.set(TEMPLATE + "user-limit-factor", "3");
+ csConf.set(TEMPLATE + "user-settings.u1."
+ + CapacitySchedulerConfiguration.USER_WEIGHT, "0.5");
+
+ RMNodeLabelsManager mgr = new NullRMNodeLabelsManager();
+ mgr.init(csConf);
+ mockRM = new MockRM(csConf) {
+ protected RMNodeLabelsManager createNodeLabelManager() {
+ return mgr;
+ }
+ };
+ cs = (CapacityScheduler) mockRM.getResourceScheduler();
+ mockRM.start();
+ cs.start();
+ }
+
+ @AfterEach
+ public void tearDown() {
+ if (mockRM != null) {
+ mockRM.stop();
+ }
+ }
+
+ @Test
+ public void testTemplateWritesInvisibleToSnapshotReaders() throws Exception {
+ ConfigSnapshot snapshotBefore = cs.getQueueContext().getConfigSnapshot();
+ AbstractLeafQueue leaf = cs.getCapacitySchedulerQueueManager()
+ .createQueue(new QueuePath(AUTO));
+ assertTemplateVisibility(leaf);
+ assertSame(snapshotBefore, cs.getQueueContext().getConfigSnapshot());
+
+ // A refresh installs a new snapshot, taken before the dynamic queue is set
+ // up again, so the template writes stay invisible to it.
+ cs.reinitialize(csConf, mockRM.getRMContext());
+ assertNotSame(snapshotBefore, cs.getQueueContext().getConfigSnapshot());
+ assertTemplateVisibility((AbstractLeafQueue) cs.getQueue(AUTO));
+ }
+
+ private void assertTemplateVisibility(AbstractLeafQueue leaf) {
+ CapacitySchedulerConfiguration queueConf =
+ cs.getQueueContext().getConfiguration();
+ ConfigSnapshot snapshot = cs.getQueueContext().getConfigSnapshot();
+ String userWeightKey = AUTO_PREFIX + "user-settings.u1."
+ + CapacitySchedulerConfiguration.USER_WEIGHT;
+
+ // The template entries are written into the queue context configuration
+ assertEquals("3", queueConf.get(AUTO_PREFIX + "user-limit-factor"));
+ assertEquals("0.5", queueConf.get(userWeightKey));
+ // but not into its snapshot
+ assertSame(snapshot, queueConf.getConfigSnapshot());
+ assertNull(snapshot.get(AUTO_PREFIX + "user-limit-factor"));
+ assertNull(snapshot.get(userWeightKey));
+
+ // The getter reads the configuration: the template value overrides the
+ // dynamic leaf default of -1, which is written before the templates and
+ // therefore does not count as explicitly set.
+ assertEquals(3f, leaf.getUserLimitFactor(), 1e-6);
+ // User weights are read through the snapshot and miss the template value
+ assertEquals(UserWeights.DEFAULT_WEIGHT,
+ leaf.getUserWeights().getByUser("u1"), 1e-6);
Review Comment:
This test asserts that a v2 template user weight
(`…auto-queue-creation-v2.template.user-settings.u1.weight`) is *not* applied
to the dynamic leaf. I ran the same scenario on the PR's base commit and trunk
behaves the same, so this PR doesn't introduce it. But CapacityScheduler.md
says v2 templates accept any queue property, and `user-settings.<user>.weight`
is one, so it looks like an existing bug.
Since this test now runs and passes in precommit, it would turn the bug into
expected behaviour: whoever fixes it later will see this test fail and may read
that as a regression. Could we file a JIRA and either drop this assertion or
mark it as known behaviour with a TODO linking that JIRA?
--
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]
