9uapaw commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r934582154
##########
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 +59,17 @@ public ConfigurationProperties(Map<String, String> 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.
+ * @param props properties to store
+ * @param whiteListPrefix only those properties will be in the nodes
+ * which starts with one of the provided prefixes.
+ */
+ public ConfigurationProperties(Map<String, String> props, String...
whiteListPrefix) {
+ this(Maps.filterKeys(props, key -> StringUtils.startsWithAny(key,
whiteListPrefix)));
Review Comment:
Using lambdas is an inferior solution in terms of performance. It is
generally discouraged to use them in Hadoop except in cases where
1. Performance does not matter (like tests).
2. Lambda-based solution is complexity-wise much better and extendable,
easier to reason about.
##########
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 +109,23 @@ public Map<String, String> getPropertiesWithPrefix(
return properties;
}
+ /**
+ * Update or create value in the nodes
+ * @param name name of the property
+ * @param value value of the property
+ */
+ public void set(String name, String value) {
+ getNode(name).ifPresent(node -> node.getValues().put(name, value));
+ }
+
+ /**
+ * Delete value from nodes
+ * @param name name of the property
+ */
+ public void unset(String name) {
+ getNode(name).ifPresent(node -> node.getValues().remove(name));
Review Comment:
Same here.
##########
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:
##########
@@ -1200,7 +1207,23 @@ public ConfigurationProperties
getConfigurationProperties() {
public void reinitializeConfigurationProperties() {
// Props are always Strings, therefore this cast is safe
Map<String, String> props = (Map) getProps();
- configurationProperties = new ConfigurationProperties(props);
+ configurationProperties = new ConfigurationProperties(props, PREFIX);
+ }
+
+ @Override
+ public void set(String name, String value) {
+ super.set(name, value);
+ if (configurationProperties != null) {
Review Comment:
Please use the getConfigurationProperties getter here.
##########
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 +109,23 @@ public Map<String, String> getPropertiesWithPrefix(
return properties;
}
+ /**
+ * Update or create value in the nodes
+ * @param name name of the property
+ * @param value value of the property
+ */
+ public void set(String name, String value) {
+ getNode(name).ifPresent(node -> node.getValues().put(name, value));
Review Comment:
Same here.
##########
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,14 +190,24 @@ private void copyProperties(
*/
private void storePropertiesInPrefixNodes(Map<String, String> props) {
for (Map.Entry<String, String> prop : props.entrySet()) {
- List<String> propertyKeyParts = splitPropertyByDelimiter(prop.getKey());
- if (!propertyKeyParts.isEmpty()) {
- PrefixNode node = findOrCreatePrefixNode(nodes,
- propertyKeyParts.iterator());
- node.getValues().put(prop.getKey(), prop.getValue());
- } else {
- LOG.warn("Empty configuration property, skipping...");
- }
+ getNode(prop.getKey()).ifPresent(
+ node -> node.getValues().put(prop.getKey(), prop.getValue()));
+ }
+ }
+
+ /**
+ * 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 empty
optional will return
+ */
+ private Optional<PrefixNode> getNode(String name) {
Review Comment:
Optional is only ergonomic when used in tandem with the Streaming API. I
think we are better off with the classic solution here (explicit nulls).
##########
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 +109,23 @@ public Map<String, String> getPropertiesWithPrefix(
return properties;
}
+ /**
+ * Update or create value in the nodes
+ * @param name name of the property
+ * @param value value of the property
+ */
+ public void set(String name, String value) {
+ getNode(name).ifPresent(node -> node.getValues().put(name, value));
+ }
+
+ /**
+ * Delete value from nodes
+ * @param name name of the property
+ */
+ public void unset(String name) {
+ getNode(name).ifPresent(node -> node.getValues().remove(name));
Review Comment:
What happens when a node does not have any more children or values left? We
need some sort of garbage collection here to avoid memory leaks.
##########
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:
##########
@@ -1200,7 +1207,23 @@ public ConfigurationProperties
getConfigurationProperties() {
public void reinitializeConfigurationProperties() {
// Props are always Strings, therefore this cast is safe
Map<String, String> props = (Map) getProps();
- configurationProperties = new ConfigurationProperties(props);
+ configurationProperties = new ConfigurationProperties(props, PREFIX);
+ }
+
+ @Override
+ public void set(String name, String value) {
+ super.set(name, value);
+ if (configurationProperties != null) {
+ configurationProperties.set(name, value);
+ }
+ }
+
+ @Override
+ public void unset(String name) {
+ super.unset(name);
+ if (configurationProperties != null) {
Review Comment:
Same here.
--
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]
