[GitHub] [hudi] yihua commented on a diff in pull request #7869: [HUDI-5713] created essential property for configs

["yihua (via GitHub)" <gi...@apache.org>]

yihua commented on code in PR #7869:
URL: https://github.com/apache/hudi/pull/7869#discussion_r1128343130


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -207,6 +207,7 @@ public class HoodieWriteConfig extends HoodieConfig {
   public static final ConfigProperty<String> BASE_PATH = ConfigProperty
       .key("hoodie.base.path")
       .noDefaultValue()
+      .makeEssential(true)

Review Comment:
   IMO, by defualt, the `essential` property should be false, meaning that the config has to be marked "essential" explicitly, enforcing us to think twice when introducing a new essential config.  For this API, we can remove the argument, i.e., `.makeEssential()` instead of `.makeEssential(true)`.



##########
hudi-common/src/main/java/org/apache/hudi/common/config/ConfigProperty.java:
##########
@@ -127,34 +130,42 @@ public List<String> getAlternatives() {
     return Arrays.asList(alternatives);
   }
 
+  public boolean isEssential() {
+    return essential;
+  }
+
   public ConfigProperty<T> withDocumentation(String doc) {
     Objects.requireNonNull(doc);
-    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, inferFunction, validValues, alternatives);
+    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, inferFunction, validValues, essential, alternatives);
   }
 
   public ConfigProperty<T> withValidValues(String... validValues) {
     Objects.requireNonNull(validValues);
-    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, inferFunction, new HashSet<>(Arrays.asList(validValues)), alternatives);
+    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, inferFunction, new HashSet<>(Arrays.asList(validValues)), essential, alternatives);
   }
 
   public ConfigProperty<T> withAlternatives(String... alternatives) {
     Objects.requireNonNull(alternatives);
-    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, inferFunction, validValues, alternatives);
+    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, inferFunction, validValues, essential, alternatives);
   }
 
   public ConfigProperty<T> sinceVersion(String sinceVersion) {
     Objects.requireNonNull(sinceVersion);
-    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, Option.of(sinceVersion), deprecatedVersion, inferFunction, validValues, alternatives);
+    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, Option.of(sinceVersion), deprecatedVersion, inferFunction, validValues, essential, alternatives);
   }
 
   public ConfigProperty<T> deprecatedAfter(String deprecatedVersion) {
     Objects.requireNonNull(deprecatedVersion);
-    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, Option.of(deprecatedVersion), inferFunction, validValues, alternatives);
+    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, Option.of(deprecatedVersion), inferFunction, validValues, essential, alternatives);
   }
 
   public ConfigProperty<T> withInferFunction(Function<HoodieConfig, Option<T>> inferFunction) {
     Objects.requireNonNull(inferFunction);
-    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, Option.of(inferFunction), validValues, alternatives);
+    return new ConfigProperty<>(key, defaultValue, docOnDefaultValue, doc, sinceVersion, deprecatedVersion, Option.of(inferFunction), validValues, essential, alternatives);
+  }
+
+  public ConfigProperty<T> makeEssential(boolean isEssential) {

Review Comment:
   As mentioned above, change this to `makeEssential()` without taking any argument.



##########
hudi-common/src/main/java/org/apache/hudi/common/config/ConfigProperty.java:
##########
@@ -171,9 +182,9 @@ public static PropertyBuilder key(String key) {
   @Override
   public String toString() {
     return String.format(
-        "Key: '%s' , default: %s description: %s since version: %s deprecated after: %s)",
+        "Key: '%s' , default: %s description: %s since version: %s deprecated after: %s , is essential: %s)",

Review Comment:
   nit: add this before `description: %s` since the description can be long?



##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -91,11 +91,13 @@ public class HoodieTableConfig extends HoodieConfig {
   public static final ConfigProperty<String> NAME = ConfigProperty
       .key(HOODIE_TABLE_NAME_KEY)
       .noDefaultValue()
+      .makeEssential(true)
       .withDocumentation("Table name that will be used for registering with Hive. Needs to be same across runs.");
 
   public static final ConfigProperty<HoodieTableType> TYPE = ConfigProperty
       .key("hoodie.table.type")
       .defaultValue(HoodieTableType.COPY_ON_WRITE)
+      .makeEssential(true)

Review Comment:
   Remove these since these are not supposed to be tweaked by the user?



-- 
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: commits-unsubscribe@hudi.apache.org

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