[GitHub] [accumulo] dlmarion commented on a diff in pull request #2695: Minor little cleanups

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

dlmarion commented on code in PR #2695:
URL: https://github.com/apache/accumulo/pull/2695#discussion_r872375137


##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1828,24 +1785,24 @@ public static <T> T createInstanceFromPropertyName(AccumuloConfiguration conf, P
     // Precomputing information here avoids :
     // * Computing it each time a method is called
     // * Using synch to compute the first time a method is called
-    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());
-      }
-      // exclude prefix types (prevents setting a prefix type like table.custom or
-      // table.constraint, directly, since they aren't valid properties on their own)
-      if (!p.getType().equals(PropertyType.PREFIX)
-          && p.getKey().startsWith(Property.TABLE_PREFIX.getKey())) {
-        validTableProperties.add(p.getKey());
-      }
-    }
+    Predicate<Property> isPrefix = p -> p.getType() == PropertyType.PREFIX;
+    Arrays.stream(Property.values())
+        // record all properties by key
+        .peek(p -> propertiesByKey.put(p.getKey(), p))
+        // save all the prefix properties
+        .peek(p -> {
+          if (isPrefix.test(p))
+            validPrefixes.add(p.getKey());
+        })
+        // only use the keys for the non-prefix properties from here on
+        .filter(isPrefix.negate()).map(Property::getKey)
+        // everything left is a valid property
+        .peek(validProperties::add)
+        // but some are also valid table properties
+        .filter(k -> k.startsWith(Property.TABLE_PREFIX.getKey()))
+        .forEach(validTableProperties::add);

Review Comment:
   I don't understand the impetus to change everything to use streams. When looking for information about the performance of for-loops vs streams it seems the consensus is that for-loops are faster unless you have a small set of data to iterate over or you are using parallel streams.



##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1581,76 +1578,35 @@ public static boolean isSensitive(String key) {
     Property prop = propertiesByKey.get(key);
     if (prop != null) {
       return prop.isSensitive();
-    } else {
-      for (String prefix : validPrefixes) {
-        if (key.startsWith(prefix)) {
-          if (propertiesByKey.get(prefix).isSensitive()) {
-            return true;
-          }
-        }
-      }
     }
-    return false;
+    return validPrefixes.stream().filter(key::startsWith).map(propertiesByKey::get)
+        .anyMatch(Property::isSensitive);
   }
 
   private <T extends Annotation> boolean hasAnnotation(Class<T> annotationType) {
-    Logger log = LoggerFactory.getLogger(getClass());
-    try {
-      for (Annotation a : getClass().getField(name()).getAnnotations()) {
-        if (annotationType.isInstance(a)) {
-          return true;
-        }
-      }
-    } catch (SecurityException | NoSuchFieldException e) {
-      log.error("{}", e.getMessage(), e);
-    }
-    return false;
+    return getAnnotation(annotationType) != null;
   }
 
   private <T extends Annotation> T getAnnotation(Class<T> annotationType) {
-    Logger log = LoggerFactory.getLogger(getClass());
     try {
-      for (Annotation a : getClass().getField(name()).getAnnotations()) {
-        if (annotationType.isInstance(a)) {
-          @SuppressWarnings("unchecked")
-          T uncheckedA = (T) a;
-          return uncheckedA;
-        }
-      }
+      return getClass().getField(name()).getAnnotation(annotationType);
     } catch (SecurityException | NoSuchFieldException e) {
-      log.error("{}", e.getMessage(), e);
+      LoggerFactory.getLogger(getClass()).error("{}", e.getMessage(), e);

Review Comment:
   We should create a static final Logger in the class.



-- 
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