[GitHub] [accumulo] ctubbsii commented on a change in pull request #2186: Refactor common table property validation to reduce duplication

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

ctubbsii commented on a change in pull request #2186:
URL: https://github.com/apache/accumulo/pull/2186#discussion_r662524342



##########
File path: core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java
##########
@@ -186,4 +188,61 @@ public void testIsValidTablePropertyKey() {
 
     assertFalse(Property.isValidTablePropertyKey("abc.def"));
   }
+
+  /**
+   * PR #2186 eliminated a loop in the computation - this test verifies the original methods and the
+   * refactor method produce the same result.
+   */
+  @Test
+  public void verifyRefactor() {
+

Review comment:
       I'm not sure I follow what this test is trying to do. Based on the name, it seems like a one-off sanity check during development. What would a regression in this test mean in the future? Based on the name and description, it seems like it would imply that a previous refactor was somehow breaking, even though that refactor had already happened in the past.
   
   If the test still has value for current/contemporary code after the refactor, the name should reflect what it is currently testing, well after this refactor is completed. If not, it should be deleted.

##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1531,21 +1547,19 @@ public static boolean isClassProperty(String key) {
     propertiesByKey = new HashMap<>();
     validPrefixes = new HashSet<>();
     validProperties = new HashSet<>();
+    validTableProperties = new HashSet<>();

Review comment:
       These don't even need to be assigned here. These 4 lines could be deleted, if the `= new HashMap<>();` part were appended to their respective `private static final` lines instead of just removing the `= null;` and assigning them here.

##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1531,21 +1547,19 @@ public static boolean isClassProperty(String key) {
     propertiesByKey = new HashMap<>();
     validPrefixes = new HashSet<>();
     validProperties = new HashSet<>();
+    validTableProperties = new HashSet<>();
 
     for (Property p : Property.values()) {
+      propertiesByKey.put(p.getKey(), p);
       if (p.getType().equals(PropertyType.PREFIX)) {
         validPrefixes.add(p.getKey());
       } else {
+

Review comment:
       Not sure how this made it in. It seems a few blank lines made their way in. It's not a big deal, but the changeset is smaller if these are omitted when not needed.

##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1531,21 +1547,19 @@ public static boolean isClassProperty(String key) {
     propertiesByKey = new HashMap<>();
     validPrefixes = new HashSet<>();
     validProperties = new HashSet<>();
+    validTableProperties = new HashSet<>();
 
     for (Property p : Property.values()) {
+      propertiesByKey.put(p.getKey(), p);

Review comment:
       I can't tell if this got moved for aesthetics, or some other reason. Does it behave the same here as it did before, when it was below the if/else?

##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -1531,21 +1547,19 @@ public static boolean isClassProperty(String key) {
     propertiesByKey = new HashMap<>();
     validPrefixes = new HashSet<>();
     validProperties = new HashSet<>();
+    validTableProperties = new HashSet<>();
 
     for (Property p : Property.values()) {
+      propertiesByKey.put(p.getKey(), p);
       if (p.getType().equals(PropertyType.PREFIX)) {
         validPrefixes.add(p.getKey());
       } else {
+
         validProperties.add(p.getKey());
-      }
-      propertiesByKey.put(p.getKey(), p);
-    }
 
-    validTableProperties = new HashSet<>();
-    for (Property p : Property.values()) {
-      if (!p.getType().equals(PropertyType.PREFIX)
-          && p.getKey().startsWith(Property.TABLE_PREFIX.getKey())) {
-        validTableProperties.add(p.getKey());
+        if (p.getKey().startsWith(Property.TABLE_PREFIX.getKey())) {
+          validTableProperties.add(p.getKey());
+        }

Review comment:
       I like that this logic was moved into the previous loop to avoid looping twice... although I don't think it will make much of a difference since it's a static initializer that will only run once and there aren't that many to loop through.
   
   However, it's a problem that the logic has changed to no longer exclude PREFIX types that start with "table." This structure should not contain prefix types, but only actual keys. We don't want users to be able to set `table.custom.` or `table.constraint.` by themselves because these are incorrectly deemed to be valid properties when they aren't.




-- 
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: notifications-unsubscribe@accumulo.apache.org

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