[GitHub] [hadoop] K0K0V0K commented on a diff in pull request #4655: YARN-11216. Avoid unnecessary reconstruction of ConfigurationProperties

2023-01-09 Thread GitBox 


K0K0V0K commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r1064638015


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerConfiguration.java:
##
@@ -107,6 +110,40 @@ public void testDefaultSubmitACLForRootAllAllowed() {
 assertTrue(acl.isAllAllowed());
   }
 
+  /**
+   * dfs.nfs.exports.allowed.hosts
+   * prop is deprecated, now we use
+   * nfs.exports.allowed.hosts
+   * instead
+   */
+  @Test
+  public void testDeprecationFeatureWorks() {

Review Comment:
   When we set a deprecated properite, there is some logic under the hood what 
keeps the old value and add an alias to the properties object, and we have to 
ensure the alias add also present in the tree.
   
   The unset unfortunately not works fine, but i created a ticket for that
   https://issues.apache.org/jira/browse/YARN-11348



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] K0K0V0K commented on a diff in pull request #4655: YARN-11216. Avoid unnecessary reconstruction of ConfigurationProperties

2023-01-09 Thread GitBox 


K0K0V0K commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r1064630413


##
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.
+   * @param name name of the property
+   * @param value value of the property
+   */
+  public void set(String name, String value) {
+PrefixNode node = getNode(name);

Review Comment:
   if the node is null, than the getNode will create a WARNING message
   ```
 /**
  * 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
  */
 private PrefixNode getNode(String name) {
   List propertyKeyParts = splitPropertyByDelimiter(name);
   if (!propertyKeyParts.isEmpty()) {
 return findOrCreatePrefixNode(null, propertyKeyParts.iterator());
   } else {
 LOG.warn("Empty configuration property");
 return null;
   }
 }
   ```



##
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.
+   * @param name name of the property
+   * @param value value of the property
+   */
+  public void set(String name, String value) {
+PrefixNode node = getNode(name);
+if (node != null) {
+  node.getValues().put(name, value);
+}
+  }
+
+  /**
+   * Delete value from nodes.
+   * @param name name of the property
+   */
+  public void unset(String name) {
+PrefixNode node = getNode(name);
+if (node != null) {

Review Comment:
   if the node is null, than the getNode will create a WARNING message
   ```
 /**
  * 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
  */
 private PrefixNode getNode(String name) {
   List propertyKeyParts = splitPropertyByDelimiter(name);
   if (!propertyKeyParts.isEmpty()) {
 return findOrCreatePrefixNode(null, propertyKeyParts.iterator());
   } else {
 LOG.warn("Empty configuration property");
 return null;
   }
 }
   ```



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] K0K0V0K commented on a diff in pull request #4655: YARN-11216. Avoid unnecessary reconstruction of ConfigurationProperties

2022-10-13 Thread GitBox 


K0K0V0K commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r994719147


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerConfiguration.java:
##
@@ -107,6 +110,40 @@ public void testDefaultSubmitACLForRootAllAllowed() {
 assertTrue(acl.isAllAllowed());
   }
 
+  /**
+   * dfs.nfs.exports.allowed.hosts
+   * prop is deprecated, new we use

Review Comment:
   typo



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] K0K0V0K commented on a diff in pull request #4655: YARN-11216. Avoid unnecessary reconstruction of ConfigurationProperties

2022-10-12 Thread GitBox 


K0K0V0K commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r993197109


##
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,24 @@ public ConfigurationProperties 
getConfigurationProperties() {
   public void reinitializeConfigurationProperties() {
 // Props are always Strings, therefore this cast is safe
 Map 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) {
+  //The super#get method used cause the super#set contains some logic
+  configurationProperties.set(super.get(name), value);

Review Comment:
   I think we should keep them in sync, cause if later someone wants to use the 
deprecation feature here, than a bug can be easily made



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] K0K0V0K commented on a diff in pull request #4655: YARN-11216. Avoid unnecessary reconstruction of ConfigurationProperties

2022-10-05 Thread GitBox 


K0K0V0K commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r985932320


##
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 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:
   The getter will initialise the tree if it is not present in the object, 
based on the configuration.
   If we set values on this object but wont use the tree view of the data, then 
the tree building will be wasted.
   Why we should use the getter 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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] K0K0V0K commented on a diff in pull request #4655: YARN-11216. Avoid unnecessary reconstruction of ConfigurationProperties

2022-10-05 Thread GitBox 


K0K0V0K commented on code in PR #4655:
URL: https://github.com/apache/hadoop/pull/4655#discussion_r985825269


##
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 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 props, String... 
whiteListPrefix) {
+this(Maps.filterKeys(props, key -> StringUtils.startsWithAny(key, 
whiteListPrefix)));

Review Comment:
   Thanks for the review @9uapaw and sorry for the late response



##
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 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 props, String... 
whiteListPrefix) {
+this(Maps.filterKeys(props, key -> StringUtils.startsWithAny(key, 
whiteListPrefix)));

Review Comment:
   Thanks for the review @9uapaw and sorry for the late response
   I dont think the lambda has performance issue here. I created a small perf 
test to prove it, and based on it the vanilia solution is way slower than 
guava. I suggest the Hashmap.put() method requires some mem allocation time 
when the map reach the limit.
   
   
[benchmark_code.txt](https://github.com/apache/hadoop/files/9698166/benchmark_code.txt)
   
[benchmark_log.txt](https://github.com/apache/hadoop/files/9698167/benchmark_log.txt)
   



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org