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

[GitBox <gi...@apache.org>]

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


##########
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<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) {
+      //The super#get method used cause the super#set contains some logic
+      configurationProperties.set(super.get(name), value);

Review Comment:
   I don't think that this is enough. The set may create multiple entries in the configuration (deprecation handling logic and alternative names), the get() just returns one (last?). (The documentation lies, it says "first"):
   
   ```
     /** 
      * Get the value of the <code>name</code>. If the key is deprecated,
      * it returns the value of the first key which replaces the deprecated key
      * and is not null.
      * If no such property exists,
      * then <code>defaultValue</code> is returned.
      * 
      * @param name property name, will be trimmed before get value.
      * @param defaultValue default value.
      * @return property value, or <code>defaultValue</code> if the property 
      *         doesn't exist.                    
      */
     public String get(String name, String defaultValue) {
       String[] names = handleDeprecation(deprecationContext.get(), name);
       String result = null;
       for(String n : names) {
         result = substituteVars(getProps().getProperty(n, defaultValue));
       }
       return result;
     }
   ```
   
   The Configuration class is marked as stable API, so it's hard to change these methods. I'm not sure if there is any way to know which configs were set during this call other than copying the object then comparing it the the altered.
   
   Need to investigate if this is even feasible (keeping everything in sync).



-- 
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-unsubscribe@hadoop.apache.org

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


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