[ALLUXIO-3219] Consolidate report configuration with getConf (#7396)

* Consolidate report configuration with getConf

* Address comments

* Address comments to use dependency-injection to test

* Fix style check

* Fix style check

core/client/fs/src/main/java/alluxio/client/RetryHandlingMetaMasterClient.java

@@ -46,7 +46,7 @@
  * support for retries.
  */
 @ThreadSafe
-public final class RetryHandlingMetaMasterClient extends AbstractMasterClient
+public class RetryHandlingMetaMasterClient extends AbstractMasterClient
     implements MetaMasterClient {
   private MetaMasterClientService.Client mClient;
 

core/common/src/main/java/alluxio/PropertyKey.java

@@ -105,6 +105,7 @@ public static final class Builder {
     private String mDescription;
     private String mName;
     private boolean mIgnoredSiteProperty;
+    private boolean mIsBuiltIn = true;
     private boolean mIsHidden;
     private ConsistencyCheckLevel mConsistencyCheckLevel = ConsistencyCheckLevel.IGNORE;
     private Scope mScope = Scope.ALL;
@@ -180,6 +181,15 @@ public Builder setDescription(String description) {
       return this;
     }
 
+    /**
+     * @param isBuiltIn whether to the property is a built-in Alluxio property
+     * @return the updated builder instance
+     */
+    public Builder setIsBuiltIn(boolean isBuiltIn) {
+      mIsBuiltIn = isBuiltIn;
+      return this;
+    }
+
     /**
      * @param isHidden whether to hide the property when generating property documentation
      * @return the updated builder instance
@@ -252,7 +262,8 @@ private PropertyKey buildUnregistered() {
       }
 
       PropertyKey key = new PropertyKey(mName, mDescription, defaultSupplier, mAlias,
-          mIgnoredSiteProperty, mIsHidden, mConsistencyCheckLevel, mScope, mDisplayType);
+          mIgnoredSiteProperty, mIsHidden, mConsistencyCheckLevel, mScope, mDisplayType,
+          mIsBuiltIn);
       return key;
     }
 
@@ -366,19 +377,15 @@ public String toString() {
           .build();
   public static final PropertyKey SITE_CONF_DIR =
       new Builder(Name.SITE_CONF_DIR)
-          .setDefaultValue(
+          .setDefaultSupplier(
+              () -> String.format("${%s}/,%s/.alluxio/,/etc/alluxio/",
+                  Name.CONF_DIR, System.getProperty("user.home")),
               String.format("${%s}/,${user.home}/.alluxio/,/etc/alluxio/", Name.CONF_DIR))
           .setDescription(
               String.format("Comma-separated search path for %s.", Constants.SITE_PROPERTIES))
           .setIgnoredSiteProperty(true)
           .setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
           .build();
-  public static final PropertyKey USER_HOME =
-      new Builder(Name.USER_HOME)
-          .setIsHidden(true)
-          .setDescription("The user.home system property")
-          .setDefaultValue("boop")
-          .build();
   public static final PropertyKey TEST_MODE =
       new Builder(Name.TEST_MODE)
           .setDefaultValue(false)
@@ -3041,7 +3048,6 @@ public static final class Name {
         "alluxio.network.thrift.frame.size.bytes.max";
     public static final String SITE_CONF_DIR = "alluxio.site.conf.dir";
     public static final String TEST_MODE = "alluxio.test.mode";
-    public static final String USER_HOME = "user.home";
     public static final String TMP_DIRS = "alluxio.tmp.dirs";
     public static final String VERSION = "alluxio.version";
     public static final String WEB_RESOURCES = "alluxio.web.resources";
@@ -3777,6 +3783,9 @@ public static Collection<? extends PropertyKey> defaultKeys() {
   /** Whether to ignore as a site property. */
   private final boolean mIgnoredSiteProperty;
 
+  /** Whether the property is an Alluxio built-in property. */
+  private final boolean mIsBuiltIn;
+
   /** Whether to hide in document. */
   private final boolean mIsHidden;
 
@@ -3796,12 +3805,16 @@ public static Collection<? extends PropertyKey> defaultKeys() {
    * @param aliases alias of this property key
    * @param ignoredSiteProperty true if Alluxio ignores user-specified value for this property in
    *        site properties file
+   * @param isHidden whether to hide in document
    * @param consistencyCheckLevel the consistency check level to apply to this property
    * @param scope the scope this property applies to
+   * @param displayType how the property value should be displayed
+   * @param isBuiltIn whether this is an Alluxio built-in property
    */
   private PropertyKey(String name, String description, DefaultSupplier defaultSupplier,
       String[] aliases, boolean ignoredSiteProperty, boolean isHidden,
-      ConsistencyCheckLevel consistencyCheckLevel, Scope scope, DisplayType displayType) {
+      ConsistencyCheckLevel consistencyCheckLevel, Scope scope, DisplayType displayType,
+      boolean isBuiltIn) {
     mName = Preconditions.checkNotNull(name, "name");
     // TODO(binfan): null check after we add description for each property key
     mDescription = Strings.isNullOrEmpty(description) ? "N/A" : description;
@@ -3812,14 +3825,15 @@ private PropertyKey(String name, String description, DefaultSupplier defaultSupp
     mConsistencyCheckLevel = consistencyCheckLevel;
     mScope = scope;
     mDisplayType = displayType;
+    mIsBuiltIn = isBuiltIn;
   }
 
   /**
    * @param name String of this property
    */
   private PropertyKey(String name) {
     this(name, null, new DefaultSupplier(() -> null, "null"), null, false, false,
-        ConsistencyCheckLevel.IGNORE, Scope.ALL, DisplayType.DEFAULT);
+        ConsistencyCheckLevel.IGNORE, Scope.ALL, DisplayType.DEFAULT, true);
   }
 
   /**
@@ -3833,7 +3847,9 @@ public static boolean register(PropertyKey key) {
     String name = key.getName();
     String[] aliases = key.getAliases();
     if (DEFAULT_KEYS_MAP.containsKey(name)) {
-      return false;
+      if (DEFAULT_KEYS_MAP.get(name).isBuiltIn() || !key.isBuiltIn()) {
+        return false;
+      }
     }
     DEFAULT_KEYS_MAP.put(name, key);
     if (aliases != null) {
@@ -3942,6 +3958,13 @@ public boolean isIgnoredSiteProperty() {
     return mIgnoredSiteProperty;
   }
 
+  /**
+   * @return true if this property is built-in
+   */
+  public boolean isBuiltIn() {
+    return mIsBuiltIn;
+  }
+
   /**
    * @return true if this property should not show up in the document
    */

core/common/src/main/java/alluxio/conf/AlluxioProperties.java

@@ -133,7 +133,7 @@ public void merge(Map<?, ?> properties, Source source) {
         // This will register the key as a valid PropertyKey
         // TODO(adit): Do not add properties unrecognized by Ufs extensions when Configuration
         // is made dynamic
-        propertyKey = new PropertyKey.Builder(key).build();
+        propertyKey = new PropertyKey.Builder(key).setIsBuiltIn(false).build();
       }
       put(propertyKey, value, source);
     }

core/common/src/test/java/alluxio/ConfigurationTest.java

@@ -854,4 +854,16 @@ public void getCredentialsDisplayValueIdentical() {
         ConfigurationValueOptions.defaults().useDisplayValue(true));
     assertEquals(displayValue1, displayValue2);
   }
+
+  @Test
+  public void extensionProperty() {
+    // simulate the case a ext key is picked by site property, unrecognized
+    String fakeKeyName = "fake.extension.key";
+    Configuration.merge(ImmutableMap.of(fakeKeyName, "value"), Source.siteProperty("ignored"));
+    assertFalse(PropertyKey.fromString(fakeKeyName).isBuiltIn());
+    // simulate the case the same key is built again inside the extension
+    PropertyKey fakeExtensionKey = new PropertyKey.Builder(fakeKeyName).build();
+    assertEquals("value", Configuration.get(fakeExtensionKey));
+    assertTrue(PropertyKey.fromString(fakeKeyName).isBuiltIn());
+  }
 }

core/common/src/test/java/alluxio/PropertyKeyTest.java

@@ -261,4 +261,9 @@ public void localityTemplates() throws Exception {
 
     assertEquals("alluxio.locality.custom", Template.LOCALITY_TIER.format("custom").toString());
   }
+
+  @Test
+  public void isBuiltIn() {
+    assertTrue(mTestProperty.isBuiltIn());
+  }
 }

core/server/master/src/main/java/alluxio/master/meta/DefaultMetaMaster.java

@@ -297,9 +297,8 @@ public ConfigCheckReport getConfigCheckReport() {
   @Override
   public List<ConfigProperty> getConfiguration(GetConfigurationOptions options) {
     List<ConfigProperty> configInfoList = new ArrayList<>();
-    String alluxioConfPrefix = "alluxio";
     for (PropertyKey key : Configuration.keySet()) {
-      if (key.getName().startsWith(alluxioConfPrefix)) {
+      if (key.isBuiltIn()) {
         String source = Configuration.getSource(key).toString();
         String value = Configuration.getOrDefault(key, null,
             ConfigurationValueOptions.defaults().useDisplayValue(true)

shell/src/main/java/alluxio/cli/GetConf.java

@@ -14,9 +14,13 @@
 import alluxio.Configuration;
 import alluxio.ConfigurationValueOptions;
 import alluxio.PropertyKey;
-import alluxio.conf.Source;
+import alluxio.client.RetryHandlingMetaMasterClient;
+import alluxio.master.MasterClientConfig;
 import alluxio.util.ConfigurationUtils;
+import alluxio.util.FormatUtils;
+import alluxio.wire.ConfigProperty;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.CommandLineParser;
 import org.apache.commons.cli.DefaultParser;
@@ -25,36 +29,43 @@
 import org.apache.commons.cli.Options;
 import org.apache.commons.cli.ParseException;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.function.Supplier;
 
 /**
  * Utility for printing Alluxio configuration.
  */
 public final class GetConf {
-  private static final String USAGE = "USAGE: GetConf [--unit <arg>] [--source] [KEY]\n\n"
+  private static final String USAGE =
+      "USAGE: GetConf [--unit <arg>] [--source] [--master] [key]\n\n"
       + "GetConf prints the configured value for the given key. If the key is invalid, the "
       + "exit code will be nonzero. If the key is valid but isn't set, an empty string is printed. "
-      + "If no key is specified, all configuration is printed. If \"--unit\" option is specified, "
-      + "values of data size configuration will be converted to a quantity in the given unit."
-      + "E.g., with \"--unit KB\", a configuration value of \"4096\" will return 4, "
-      + "and with \"--unit S\", a configuration value of \"5000\" will return 5. "
-      + "Possible unit options include B, KB, MB, GB, TP, PB, MS, S, M, H, D. "
-      + "if \"--source\" option is specified, the source of configuration value will be printed.";
+      + "If no key is specified, all configuration is printed.";
 
+  private static final String MASTER_OPTION_NAME = "master";
   private static final String SOURCE_OPTION_NAME = "source";
   private static final String UNIT_OPTION_NAME = "unit";
+  private static final Option MASTER_OPTION =
+      Option.builder().required(false).longOpt(MASTER_OPTION_NAME).hasArg(false)
+          .desc("the configuration properties used by the master.").build();
   private static final Option SOURCE_OPTION =
       Option.builder().required(false).longOpt(SOURCE_OPTION_NAME).hasArg(false)
-          .desc("source of the configuration property.").build();
+          .desc("source of the configuration property will be printed.").build();
   private static final Option UNIT_OPTION =
       Option.builder().required(false).longOpt(UNIT_OPTION_NAME).hasArg(true)
-          .desc("unit of the value to return.").build();
+          .desc("unit of the value to return. Values of configuration will be converted "
+              + "to a quantity in the given unit. E.g., with \"--unit KB\", a configuration value "
+              + "of \"4096B\" will return 4, and with \"--unit S\", a configuration value of "
+              + "\"5000ms\" will return 5. Possible unit options include B, KB, MB, GB, TP, PB, "
+              + "MS, S, M, H, D. \"").build();
   private static final Options OPTIONS =
-      new Options().addOption(SOURCE_OPTION).addOption(UNIT_OPTION);
+      new Options().addOption(SOURCE_OPTION).addOption(UNIT_OPTION).addOption(MASTER_OPTION);
 
   private enum ByteUnit {
     B(1L),
@@ -127,69 +138,109 @@ public static void printHelp(String message) {
    * @return 0 on success, 1 on failures
    */
   public static int getConf(String... args) {
+    return getConfImpl(
+        () -> new RetryHandlingMetaMasterClient(MasterClientConfig.defaults()), args);
+  }
+
+  /**
+   * Implements get configuration.
+   *
+   * @param clientSupplier a functor to return a client of meta master
+   * @param args list of arguments
+   * @return 0 on success, 1 on failures
+   */
+  @VisibleForTesting
+  public static int getConfImpl(
+      Supplier<RetryHandlingMetaMasterClient> clientSupplier, String... args) {
     CommandLineParser parser = new DefaultParser();
     CommandLine cmd;
-
     try {
       cmd = parser.parse(OPTIONS, args, true /* stopAtNonOption */);
     } catch (ParseException e) {
       printHelp("Unable to parse input args: " + e.getMessage());
       return 1;
     }
     args = cmd.getArgs();
+
+    Map<String, ConfigProperty> confMap = new HashMap<>();
+    if (cmd.hasOption(MASTER_OPTION_NAME)) {
+      // load cluster-wide configuration
+      try (RetryHandlingMetaMasterClient client = clientSupplier.get()) {
+        client.getConfiguration().forEach(prop -> confMap.put(prop.getName(), prop));
+      } catch (IOException e) {
+        System.out.println("Unable to get master-side configuration: " + e.getMessage());
+        return -1;
+      }
+    } else {
+      // load local configuration
+      for (PropertyKey key : Configuration.keySet()) {
+        if (key.isBuiltIn()) {
+          confMap.put(key.getName(), new ConfigProperty()
+              .setName(key.getName())
+              .setValue(Configuration.getOrDefault(key, null,
+                  ConfigurationValueOptions.defaults().useDisplayValue(true)))
+              .setSource(Configuration.getSource(key).toString()));
+        }
+      }
+    }
+
     StringBuilder output = new StringBuilder();
     switch (args.length) {
       case 0:
-        List<PropertyKey> keys = new ArrayList<>(Configuration.keySet());
-        Collections.sort(keys, Comparator.comparing(PropertyKey::getName));
-        for (PropertyKey key : keys) {
-          String value = ConfigurationUtils.valueAsString(Configuration.getOrDefault(key, null,
-              ConfigurationValueOptions.defaults().useDisplayValue(true)));
-          output.append(String.format("%s=%s", key.getName(), value));
+        List<ConfigProperty> properties = new ArrayList<>(confMap.values());
+        properties.sort(Comparator.comparing(ConfigProperty::getName));
+        for (ConfigProperty property : properties) {
+          String value = ConfigurationUtils.valueAsString(property.getValue());
+          output.append(String.format("%s=%s", property.getName(), value));
           if (cmd.hasOption(SOURCE_OPTION_NAME)) {
-            Source source = Configuration.getSource(key);
-            output.append(String.format(" (%s)", source));
+            output.append(String.format(" (%s)", property.getSource()));
           }
           output.append("\n");
         }
         System.out.print(output.toString());
         break;
       case 1:
-        if (!PropertyKey.isValid(args[0])) {
-          printHelp(String.format("%s is not a valid configuration key", args[0]));
+        String key = args[0];
+        if (!PropertyKey.isValid(key)) {
+          printHelp(String.format("%s is not a valid configuration key", key));
+          return 1;
+        }
+        ConfigProperty property = confMap.get(key);
+        if (property == null) {
+          printHelp(String.format("%s is not found", key));
           return 1;
         }
-        PropertyKey key = PropertyKey.fromString(args[0]);
-        if (!Configuration.isSet(key)) {
+
+        if (property.getValue() == null) {
+          // value not set
           System.out.println("");
         } else {
           if (cmd.hasOption(SOURCE_OPTION_NAME)) {
-            Source source = Configuration.getSource(key);
-            System.out.println(source);
-          } else if (cmd.hasOption(UNIT_OPTION_NAME)
-              && key.getDisplayType() != PropertyKey.DisplayType.CREDENTIALS) {
+            System.out.println(property.getSource());
+          } else if (cmd.hasOption(UNIT_OPTION_NAME)) {
             String arg = cmd.getOptionValue(UNIT_OPTION_NAME).toUpperCase();
             try {
               ByteUnit byteUnit;
               byteUnit = ByteUnit.valueOf(arg);
-              System.out.println(Configuration.getBytes(key) / byteUnit.getValue());
+              System.out.println(
+                  FormatUtils.parseSpaceSize(property.getValue()) / byteUnit.getValue());
               break;
             } catch (Exception e) {
               // try next unit parse
             }
             try {
               TimeUnit timeUnit;
               timeUnit = TimeUnit.valueOf(arg);
-              System.out.println(Configuration.getMs(key) / timeUnit.getValue());
+              System.out.println(
+                  FormatUtils.parseTimeSize(property.getValue()) / timeUnit.getValue());
               break;
             } catch (IllegalArgumentException ex) {
               // try next unit parse
             }
             printHelp(String.format("%s is not a valid unit", arg));
             return 1;
           } else {
-            System.out.println(Configuration.get(key,
-                ConfigurationValueOptions.defaults().useDisplayValue(true)));
+            System.out.println(property.getValue());
           }
         }
         break;