accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ktur...@apache.org
Subject [accumulo] 01/06: Revert "ACCUMULO-4779 Speedup Property by precomputing and avoiding sync (#366)"
Date Fri, 02 Feb 2018 18:25:11 GMT
This is an automated email from the ASF dual-hosted git repository.

kturner pushed a commit to branch 1.7
in repository https://gitbox.apache.org/repos/asf/accumulo.git

commit cf0b8aa4994e10242478c4d9412d924d7e6de91c
Author: Keith Turner <kturner@apache.org>
AuthorDate: Fri Feb 2 12:11:53 2018 -0500

    Revert "ACCUMULO-4779 Speedup Property by precomputing and avoiding sync (#366)"
    
    This reverts commit 1fe3ba12a943e590b89b2979e661e7dc447d0774.
---
 .../org/apache/accumulo/core/conf/Property.java    | 154 ++++++++-------------
 .../apache/accumulo/core/conf/PropertyTest.java    |  58 --------
 2 files changed, 56 insertions(+), 156 deletions(-)

diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
index 1733124..5a32a83 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
@@ -39,8 +39,6 @@ import org.apache.commons.configuration.PropertiesConfiguration;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.google.common.base.Preconditions;
-
 public enum Property {
   // Crypto-related properties
   @Experimental
@@ -591,16 +589,7 @@ public enum Property {
 
   ;
 
-  private String key;
-  private String defaultValue;
-  private String computedDefaultValue;
-  private String description;
-  private boolean annotationsComputed = false;
-  private boolean defaultValueComputed = false;
-  private boolean isSensitive;
-  private boolean isDeprecated;
-  private boolean isExperimental;
-  private boolean isInterpolated;
+  private String key, defaultValue, description;
   private PropertyType type;
   private static final Logger log = LoggerFactory.getLogger(Property.class);
 
@@ -640,11 +629,6 @@ public enum Property {
    * @return default value
    */
   public String getDefaultValue() {
-    Preconditions.checkState(defaultValueComputed, "precomputeDefaultValue() must be called
before calling this method");
-    return computedDefaultValue;
-  }
-
-  private void precomputeDefaultValue() {
     String v;
     if (isInterpolated()) {
       PropertiesConfiguration pconf = new PropertiesConfiguration();
@@ -659,9 +643,7 @@ public enum Property {
     }
     if (this.type == PropertyType.ABSOLUTEPATH && !(v.trim().equals("")))
       v = new File(v).getAbsolutePath();
-
-    computedDefaultValue = v;
-    defaultValueComputed = true;
+    return v;
   }
 
   /**
@@ -683,8 +665,7 @@ public enum Property {
   }
 
   private boolean isInterpolated() {
-    Preconditions.checkState(annotationsComputed, "precomputeAnnotations() must be called
before calling this method");
-    return isInterpolated;
+    return hasAnnotation(Interpolated.class) || hasPrefixWithAnnotation(getKey(), Interpolated.class);
   }
 
   /**
@@ -693,8 +674,7 @@ public enum Property {
    * @return true if this property is experimental
    */
   public boolean isExperimental() {
-    Preconditions.checkState(annotationsComputed, "precomputeAnnotations() must be called
before calling this method");
-    return isExperimental;
+    return hasAnnotation(Experimental.class) || hasPrefixWithAnnotation(getKey(), Experimental.class);
   }
 
   /**
@@ -703,26 +683,21 @@ public enum Property {
    * @return true if this property is deprecated
    */
   public boolean isDeprecated() {
-    Preconditions.checkState(annotationsComputed, "precomputeAnnotations() must be called
before calling this method");
-    return isDeprecated;
+    return hasAnnotation(Deprecated.class) || hasPrefixWithAnnotation(getKey(), Deprecated.class);
   }
 
+  private volatile Boolean isSensitive = null;
+
   /**
    * Checks if this property is sensitive.
    *
    * @return true if this property is sensitive
    */
   public boolean isSensitive() {
-    Preconditions.checkState(annotationsComputed, "precomputeAnnotations() must be called
before calling this method");
-    return isSensitive;
-  }
-
-  private void precomputeAnnotations() {
-    isSensitive = hasAnnotation(Sensitive.class) || hasPrefixWithAnnotation(getKey(), Sensitive.class);
-    isDeprecated = hasAnnotation(Deprecated.class) || hasPrefixWithAnnotation(getKey(), Deprecated.class);
-    isExperimental = hasAnnotation(Experimental.class) || hasPrefixWithAnnotation(getKey(),
Experimental.class);
-    isInterpolated = hasAnnotation(Interpolated.class) || hasPrefixWithAnnotation(getKey(),
Interpolated.class);
-    annotationsComputed = true;
+    if (isSensitive == null) {
+      isSensitive = hasAnnotation(Sensitive.class) || hasPrefixWithAnnotation(getKey(), Sensitive.class);
+    }
+    return isSensitive.booleanValue();
   }
 
   /**
@@ -734,20 +709,7 @@ public enum Property {
    * @return true if property is sensitive
    */
   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 hasPrefixWithAnnotation(key, Sensitive.class);
   }
 
   private <T extends Annotation> boolean hasAnnotation(Class<T> annotationType)
{
@@ -765,21 +727,24 @@ public enum Property {
   }
 
   private static <T extends Annotation> boolean hasPrefixWithAnnotation(String key,
Class<T> annotationType) {
-    for (String prefix : validPrefixes) {
-      if (key.startsWith(prefix)) {
-        if (propertiesByKey.get(prefix).hasAnnotation(annotationType)) {
-          return true;
-        }
-      }
+    // relies on side-effects of isValidPropertyKey to populate validPrefixes
+    if (isValidPropertyKey(key)) {
+      // check if property exists on its own and has the annotation
+      if (Property.getPropertyByKey(key) != null)
+        return getPropertyByKey(key).hasAnnotation(annotationType);
+      // can't find the property, so check the prefixes
+      boolean prefixHasAnnotation = false;
+      for (String prefix : validPrefixes)
+        if (key.startsWith(prefix))
+          prefixHasAnnotation = prefixHasAnnotation || getPropertyByKey(prefix).hasAnnotation(annotationType);
+      return prefixHasAnnotation;
     }
-
     return false;
   }
 
   private static HashSet<String> validTableProperties = null;
   private static HashSet<String> validProperties = null;
   private static HashSet<String> validPrefixes = null;
-  private static HashMap<String,Property> propertiesByKey = null;
 
   private static boolean isKeyValidlyPrefixed(String key) {
     for (String prefix : validPrefixes) {
@@ -798,7 +763,20 @@ public enum Property {
    *          property key
    * @return true if key is valid (recognized, or has a recognized prefix)
    */
-  public static boolean isValidPropertyKey(String key) {
+  public synchronized static boolean isValidPropertyKey(String key) {
+    if (validProperties == null) {
+      validProperties = new HashSet<>();
+      validPrefixes = new HashSet<>();
+
+      for (Property p : Property.values()) {
+        if (p.getType().equals(PropertyType.PREFIX)) {
+          validPrefixes.add(p.getKey());
+        } else {
+          validProperties.add(p.getKey());
+        }
+      }
+    }
+
     return validProperties.contains(key) || isKeyValidlyPrefixed(key);
   }
 
@@ -811,12 +789,20 @@ public enum Property {
    *          property key
    * @return true if key is valid for a table property (recognized, or has a recognized prefix)
    */
-  public static boolean isValidTablePropertyKey(String key) {
-    return validTableProperties.contains(key)
-        || (key.startsWith(Property.TABLE_PREFIX.getKey()) && (key.startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey())
-            || key.startsWith(Property.TABLE_ITERATOR_PREFIX.getKey()) || key.startsWith(Property.TABLE_LOCALITY_GROUP_PREFIX.getKey())
-            || key.startsWith(Property.TABLE_COMPACTION_STRATEGY_PREFIX.getKey()) || key.startsWith(Property.TABLE_REPLICATION_TARGET.getKey())
|| key
-              .startsWith(Property.TABLE_ARBITRARY_PROP_PREFIX.getKey())));
+  public synchronized static boolean isValidTablePropertyKey(String key) {
+    if (validTableProperties == null) {
+      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());
+        }
+      }
+    }
+
+    return validTableProperties.contains(key) || key.startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey())
+        || key.startsWith(Property.TABLE_ITERATOR_PREFIX.getKey()) || key.startsWith(Property.TABLE_LOCALITY_GROUP_PREFIX.getKey())
+        || key.startsWith(Property.TABLE_COMPACTION_STRATEGY_PREFIX.getKey()) || key.startsWith(Property.TABLE_REPLICATION_TARGET.getKey())
+        || key.startsWith(Property.TABLE_ARBITRARY_PROP_PREFIX.getKey());
   }
 
   /**
@@ -852,7 +838,7 @@ public enum Property {
     return key.startsWith(Property.TABLE_PREFIX.getKey()) || key.startsWith(Property.TSERV_PREFIX.getKey())
|| key.startsWith(Property.LOGGER_PREFIX.getKey())
         || key.startsWith(Property.MASTER_PREFIX.getKey()) || key.startsWith(Property.GC_PREFIX.getKey())
         || key.startsWith(Property.MONITOR_PREFIX.getKey() + "banner.") || key.startsWith(VFS_CONTEXT_CLASSPATH_PROPERTY.getKey())
-        || key.startsWith(REPLICATION_PREFIX.getKey());
+        || key.startsWith(Property.TABLE_COMPACTION_STRATEGY_PREFIX.getKey()) || key.startsWith(REPLICATION_PREFIX.getKey());
   }
 
   /**
@@ -863,7 +849,10 @@ public enum Property {
    * @return property, or null if not found
    */
   public static Property getPropertyByKey(String key) {
-    return propertiesByKey.get(key);
+    for (Property prop : Property.values())
+      if (prop.getKey().equals(key))
+        return prop;
+    return null;
   }
 
   /**
@@ -963,35 +952,4 @@ public enum Property {
     }
     return result;
   }
-
-  static {
-    // Precomputing information here avoids :
-    // * Computing it each time a method is called
-    // * Using synch to compute the first time a method is called
-    propertiesByKey = new HashMap<>();
-    validPrefixes = new HashSet<>();
-    validProperties = new HashSet<>();
-
-    for (Property p : Property.values()) {
-      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());
-      }
-    }
-
-    // order is very important here the following code relies on the maps and sets populated
above
-    for (Property p : Property.values()) {
-      p.precomputeAnnotations();
-      p.precomputeDefaultValue();
-    }
-  }
 }
diff --git a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java
index d25c8c2..6848ee4 100644
--- a/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java
@@ -19,7 +19,6 @@ package org.apache.accumulo.core.conf;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
@@ -153,61 +152,4 @@ public class PropertyTest {
   public void testGCDeadServerWaitSecond() {
     assertEquals("1h", Property.GC_WAL_DEAD_SERVER_WAIT.getDefaultValue());
   }
-
-  @SuppressWarnings("deprecation")
-  private Property getDeprecatedProperty() {
-    return Property.INSTANCE_DFS_DIR;
-  }
-
-  @Test
-  public void testAnnotations() {
-    assertTrue(Property.TABLE_VOLUME_CHOOSER.isExperimental());
-    assertFalse(Property.LOGGER_DIR.isExperimental());
-
-    assertTrue(Property.INSTANCE_SECRET.isSensitive());
-    assertFalse(Property.INSTANCE_VOLUMES.isSensitive());
-
-    assertTrue(getDeprecatedProperty().isDeprecated());
-    assertFalse(Property.INSTANCE_VOLUMES_REPLACEMENTS.isDeprecated());
-
-  }
-
-  @Test
-  public void testGetPropertyByKey() {
-    for (Property prop : Property.values()) {
-      assertSame(prop, Property.getPropertyByKey(prop.getKey()));
-    }
-  }
-
-  @Test
-  public void testIsValidPropertyKey() {
-    for (Property prop : Property.values()) {
-      assertTrue(Property.isValidPropertyKey(prop.getKey()));
-      if (prop.getType().equals(PropertyType.PREFIX)) {
-        assertTrue(Property.isValidPropertyKey(prop.getKey() + "foo9"));
-      }
-    }
-
-    assertFalse(Property.isValidPropertyKey("abc.def"));
-  }
-
-  @Test
-  public void testIsValidTablePropertyKey() {
-    for (Property prop : Property.values()) {
-      if (prop.getKey().startsWith("table.") && !prop.getKey().equals("table."))
{
-        assertTrue(Property.isValidTablePropertyKey(prop.getKey()));
-
-        if (prop.getType().equals(PropertyType.PREFIX)) {
-          assertTrue(Property.isValidTablePropertyKey(prop.getKey() + "foo9"));
-        } else {
-          assertFalse(Property.isValidTablePropertyKey(prop.getKey() + "foo9"));
-        }
-      } else {
-        assertFalse(Property.isValidTablePropertyKey(prop.getKey()));
-      }
-
-    }
-
-    assertFalse(Property.isValidTablePropertyKey("abc.def"));
-  }
 }

-- 
To stop receiving notification emails like this one, please contact
kturner@apache.org.

Mime
View raw message