szilard-nemeth commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r1028179146
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java:
##
@@ -242,6 +244,10 @@ public class Configuration implements
Iterable>,
private boolean restrictSystemProps = restrictSystemPropsDefault;
private boolean allowNullValueProperties = false;
+ private BiConsumer propAddListener;
Review Comment:
Can we use 'properties' as a name for fields, setters and the class
(PropertiesWithListener) without the abbreviation? I think it's not too long
and abbreviating is not really necessary in this case.
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java:
##
@@ -4060,4 +4077,26 @@ private void putIntoUpdatingResource(String key,
String[] value) {
}
localUR.put(key, value);
}
+ private class PropWithListener extends Properties {
+
+private final Configuration configuration;
+
+public PropWithListener(Configuration configuration) {
+ this.configuration = configuration;
+}
+@Override
Review Comment:
Nit: Add newline between constructor and setProperty method.
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java:
##
@@ -871,6 +877,15 @@ public Configuration(Configuration other) {
setQuietMode(other.getQuietMode());
}
+ protected synchronized void setPropListeners(
+ BiConsumer propAddListener,
+ Consumer propRemoveListener
+ ) {
+this.properties = null;
Review Comment:
Here, you could accept null values for the consumers, at least there's no
prevention for them to be null.
In getProps, the PropWithListener is created if any of the listeners are not
null (so the other one is allowed to be null).
Calling setProperty on PropWithListener is not checking if those fields are
null, which is dangerous.
Either prevent them to be null in the constructor (fail fast) or do a null
check in setProperty.
Could you please also add a testcase to cover the null scenarios?
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ConfigurationProperties.java:
##
@@ -55,6 +58,17 @@ public ConfigurationProperties(Map props) {
storePropertiesInPrefixNodes(props);
}
+ /**
+ * A constructor defined in order to conform to the type used by
+ * {@code Configuration}. It must only be called by String keys and values.
Review Comment:
```suggestion
* {@code Configuration}. It must only be called with String keys and
values.
```
##
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:
##
@@ -18,15 +18,27 @@
package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity;
-import org.apache.hadoop.classification.VisibleForTesting;
Review Comment:
Please exclude formatting (i.e. organize imports) from your commit and only
add required changes in imports.
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ConfigurationProperties.java:
##
@@ -158,39 +208,49 @@ private void copyProperties(
*/
private void storePropertiesInPrefixNodes(Map props) {
for (Map.Entry prop : props.entrySet()) {
- List propertyKeyParts = splitPropertyByDelimiter(prop.getKey());
- if (!propertyKeyParts.isEmpty()) {
-PrefixNode node = findOrCreatePrefixNode(nodes,
-propertyKeyParts.iterator());
+ PrefixNode node = getNode(prop.getKey());
+ if (node != null) {
node.getValues().put(prop.getKey(), prop.getValue());
- } else {
-LOG.warn("Empty configuration property, skipping...");
}
}
}
+ /**
+ * Finds the node that matches the whole key or create it, if it does not
exist.
+ * @param name name of the property
+ * @return the found or created node, if the name is empty, than return with
null
+ */
Review Comment:
```suggestion
* Finds the node that matches the whole key or create it if it does not
exist.
* @param name name of the property
* @return the found or newly created node, otherwise return null if the
name is empty
*/
```
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ConfigurationProperties.java:
##
@@ -94,6 +108,42 @@ public Map getPropertiesWithPrefix(
return properties;
}
+ /**
+ * Update or create value in the nodes.
+
