ACCUMULO-3012 Improve DefaultConfiguration * The internal map of resolved (default) properties is now static and immutable, making explicit synchronization unnecessary. * The main method is moved to ConfigurationDocGen. * A unit test is added. * Accompanying changes were made to ConfigSanityCheck: - The validate method takes in an Iterable of entries instead of a Configuration. - Validation failure throws a new SanityCheckException class instead of RuntimeException. - A unit test is added.
Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/37f900b7 Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/37f900b7 Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/37f900b7 Branch: refs/heads/1.6.1-SNAPSHOT Commit: 37f900b7550d6ffe7b39f2ff5cd766912ab09141 Parents: 8140804 Author: Bill Havanki <bhava...@cloudera.com> Authored: Thu Jul 24 09:36:04 2014 -0400 Committer: Bill Havanki <bhava...@cloudera.com> Committed: Wed Aug 13 10:09:57 2014 -0400 ---------------------------------------------------------------------- .../accumulo/core/conf/ConfigSanityCheck.java | 44 +++++++++-- .../accumulo/core/conf/ConfigurationDocGen.java | 19 ++++- .../core/conf/DefaultConfiguration.java | 58 ++++++-------- .../core/conf/ConfigSanityCheckTest.java | 79 ++++++++++++++++++++ .../core/conf/DefaultConfigurationTest.java | 44 +++++++++++ docs/pom.xml | 4 +- 6 files changed, 199 insertions(+), 49 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/37f900b7/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java b/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java index f9781f4..88d32ef 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/ConfigSanityCheck.java @@ -20,13 +20,24 @@ import java.util.Map.Entry; import org.apache.log4j.Logger; +/** + * A utility class for checking over configuration entries. + */ public class ConfigSanityCheck { private static final Logger log = Logger.getLogger(ConfigSanityCheck.class); private static final String PREFIX = "BAD CONFIG "; - public static void validate(AccumuloConfiguration acuconf) { - for (Entry<String,String> entry : acuconf) { + /** + * Validates the given configuration entries. + * + * @param entries iterable through configuration keys and values + * @throws SanityCheckException if a fatal configuration error is found + */ + public static void validate(Iterable<Entry<String,String>> entries) { + String instanceZkTimeoutKey = Property.INSTANCE_ZK_TIMEOUT.getKey(); + String instanceZkTimeoutValue = null; + for (Entry<String,String> entry : entries) { String key = entry.getKey(); String value = entry.getValue(); Property prop = Property.getPropertyByKey(entry.getKey()); @@ -38,9 +49,16 @@ public class ConfigSanityCheck { fatal(PREFIX + "incomplete property key (" + key + ")"); else if (!prop.getType().isValidFormat(value)) fatal(PREFIX + "improperly formatted value for key (" + key + ", type=" + prop.getType() + ")"); + + if (key.equals(instanceZkTimeoutKey)) { + instanceZkTimeoutValue = value; + } + } + + if (instanceZkTimeoutValue != null) { + checkTimeDuration(Property.INSTANCE_ZK_TIMEOUT, instanceZkTimeoutValue, + new CheckTimeDurationBetween(1000, 300000)); } - - checkTimeDuration(acuconf, Property.INSTANCE_ZK_TIMEOUT, new CheckTimeDurationBetween(1000, 300000)); } private interface CheckTimeDuration { @@ -68,9 +86,9 @@ public class ConfigSanityCheck { } } - private static void checkTimeDuration(AccumuloConfiguration acuconf, Property prop, CheckTimeDuration chk) { + private static void checkTimeDuration(Property prop, String value, CheckTimeDuration chk) { verifyPropertyTypes(PropertyType.TIMEDURATION, prop); - if (!chk.check(acuconf.getTimeInMillis(prop))) + if (!chk.check(AccumuloConfiguration.getTimeInMillis(value))) fatal(PREFIX + chk.getDescription(prop)); } @@ -79,9 +97,19 @@ public class ConfigSanityCheck { if (prop.getType() != type) fatal("Unexpected property type (" + prop.getType() + " != " + type + ")"); } - + + /** + * The exception thrown when {@link ConfigSanityCheck#validate(Iterable)} fails. + */ + public static class SanityCheckException extends RuntimeException { + /** + * Creates a new exception with the given message. + */ + public SanityCheckException(String msg) { super(msg); } + } + private static void fatal(String msg) { log.fatal(msg); - throw new RuntimeException(msg); + throw new SanityCheckException(msg); } } http://git-wip-us.apache.org/repos/asf/accumulo/blob/37f900b7/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationDocGen.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationDocGen.java b/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationDocGen.java index ca57372..41a1f3b 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationDocGen.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/ConfigurationDocGen.java @@ -16,9 +16,11 @@ */ package org.apache.accumulo.core.conf; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.io.PrintStream; +import java.io.UnsupportedEncodingException; import java.util.ArrayList; import java.util.TreeMap; @@ -277,7 +279,7 @@ class ConfigurationDocGen { } } - private static Logger log = Logger.getLogger(DefaultConfiguration.class); + private static Logger log = Logger.getLogger(ConfigurationDocGen.class); private PrintStream doc; @@ -302,7 +304,7 @@ class ConfigurationDocGen { } private void appendResource(String resourceName) { - InputStream data = DefaultConfiguration.class.getResourceAsStream(resourceName); + InputStream data = ConfigurationDocGen.class.getResourceAsStream(resourceName); if (data != null) { byte[] buffer = new byte[1024]; int n; @@ -338,4 +340,17 @@ class ConfigurationDocGen { new LaTeX().generate(); } + /* + * Generate documentation for conf/accumulo-site.xml file usage + */ + public static void main(String[] args) throws FileNotFoundException, UnsupportedEncodingException { + if (args.length == 2 && args[0].equals("--generate-html")) { + new ConfigurationDocGen(new PrintStream(args[1], Constants.UTF8.name())).generateHtml(); + } else if (args.length == 2 && args[0].equals("--generate-latex")) { + new ConfigurationDocGen(new PrintStream(args[1], Constants.UTF8.name())).generateLaTeX(); + } else { + throw new IllegalArgumentException("Usage: " + ConfigurationDocGen.class.getName() + " --generate-html <filename> | --generate-latex <filename>"); + } + } + } http://git-wip-us.apache.org/repos/asf/accumulo/blob/37f900b7/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java b/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java index 44e198a..6e1b779 100644 --- a/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java +++ b/core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java @@ -16,9 +16,7 @@ */ package org.apache.accumulo.core.conf; -import java.io.FileNotFoundException; -import java.io.PrintStream; -import java.io.UnsupportedEncodingException; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; @@ -26,51 +24,37 @@ import java.util.Map.Entry; import org.apache.accumulo.core.Constants; public class DefaultConfiguration extends AccumuloConfiguration { - private static DefaultConfiguration instance = null; - private Map<String,String> resolvedProps = null; - - synchronized public static DefaultConfiguration getInstance() { - if (instance == null) { - instance = new DefaultConfiguration(); - ConfigSanityCheck.validate(instance); + private final static Map<String,String> resolvedProps; + static { + Map<String, String> m = new HashMap<String,String>(); + for (Property prop : Property.values()) { + if (!prop.getType().equals(PropertyType.PREFIX)) { + m.put(prop.getKey(), prop.getDefaultValue()); + } } - return instance; + ConfigSanityCheck.validate(m.entrySet()); + resolvedProps = Collections.unmodifiableMap(m); } - @Override - public String get(Property property) { - return getResolvedProps().get(property.getKey()); + /** + * Gets a default configuration. + * + * @return default configuration + */ + public static DefaultConfiguration getInstance() { + return new DefaultConfiguration(); } - private synchronized Map<String,String> getResolvedProps() { - if (resolvedProps == null) { - // the following loop is super slow, it takes a few milliseconds, so cache it - resolvedProps = new HashMap<String,String>(); - for (Property prop : Property.values()) - if (!prop.getType().equals(PropertyType.PREFIX)) - resolvedProps.put(prop.getKey(), prop.getDefaultValue()); - } - return resolvedProps; + @Override + public String get(Property property) { + return resolvedProps.get(property.getKey()); } @Override public void getProperties(Map<String,String> props, PropertyFilter filter) { - for (Entry<String,String> entry : getResolvedProps().entrySet()) + for (Entry<String,String> entry : resolvedProps.entrySet()) if (filter.accept(entry.getKey())) props.put(entry.getKey(), entry.getValue()); } - /* - * Generate documentation for conf/accumulo-site.xml file usage - */ - public static void main(String[] args) throws FileNotFoundException, UnsupportedEncodingException { - if (args.length == 2 && args[0].equals("--generate-html")) { - new ConfigurationDocGen(new PrintStream(args[1], Constants.UTF8.name())).generateHtml(); - } else if (args.length == 2 && args[0].equals("--generate-latex")) { - new ConfigurationDocGen(new PrintStream(args[1], Constants.UTF8.name())).generateLaTeX(); - } else { - throw new IllegalArgumentException("Usage: " + DefaultConfiguration.class.getName() + " --generate-html <filename> | --generate-latex <filename>"); - } - } - } http://git-wip-us.apache.org/repos/asf/accumulo/blob/37f900b7/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java b/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java new file mode 100644 index 0000000..88bbe64 --- /dev/null +++ b/core/src/test/java/org/apache/accumulo/core/conf/ConfigSanityCheckTest.java @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.core.conf; + +import java.util.Map; +import org.apache.accumulo.core.conf.ConfigSanityCheck.SanityCheckException; +import org.junit.Before; +import org.junit.Test; + +public class ConfigSanityCheckTest { + private Map<String,String> m; + + @Before + public void setUp() { + m = new java.util.HashMap<String,String>(); + } + + @Test + public void testPass() { + m.put(Property.MASTER_CLIENTPORT.getKey(), "9999"); + m.put(Property.MASTER_TABLET_BALANCER.getKey(), "org.apache.accumulo.server.master.balancer.TableLoadBalancer"); + m.put(Property.MASTER_RECOVERY_MAXAGE.getKey(), "60m"); + m.put(Property.MASTER_BULK_RETRIES.getKey(), "3"); + ConfigSanityCheck.validate(m.entrySet()); + } + + @Test + public void testPass_Empty() { + ConfigSanityCheck.validate(m.entrySet()); + } + + @Test + public void testPass_UnrecognizedValidProperty() { + m.put(Property.MASTER_CLIENTPORT.getKey(), "9999"); + m.put(Property.MASTER_PREFIX.getKey() + "something", "abcdefg"); + ConfigSanityCheck.validate(m.entrySet()); + } + + @Test + public void testPass_UnrecognizedProperty() { + m.put(Property.MASTER_CLIENTPORT.getKey(), "9999"); + m.put("invalid.prefix.value", "abcdefg"); + ConfigSanityCheck.validate(m.entrySet()); + } + + @Test(expected = SanityCheckException.class) + public void testFail_Prefix() { + m.put(Property.MASTER_CLIENTPORT.getKey(), "9999"); + m.put(Property.MASTER_PREFIX.getKey(), "oops"); + ConfigSanityCheck.validate(m.entrySet()); + } + + @Test(expected = SanityCheckException.class) + public void testFail_InvalidFormat() { + m.put(Property.MASTER_CLIENTPORT.getKey(), "9999"); + m.put(Property.MASTER_RECOVERY_MAXAGE.getKey(), "60breem"); + ConfigSanityCheck.validate(m.entrySet()); + } + + @Test(expected = SanityCheckException.class) + public void testFail_InstanceZkTimeoutOutOfRange() { + m.put(Property.INSTANCE_ZK_TIMEOUT.getKey(), "10ms"); + ConfigSanityCheck.validate(m.entrySet()); + } +} http://git-wip-us.apache.org/repos/asf/accumulo/blob/37f900b7/core/src/test/java/org/apache/accumulo/core/conf/DefaultConfigurationTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/accumulo/core/conf/DefaultConfigurationTest.java b/core/src/test/java/org/apache/accumulo/core/conf/DefaultConfigurationTest.java new file mode 100644 index 0000000..beb63a7 --- /dev/null +++ b/core/src/test/java/org/apache/accumulo/core/conf/DefaultConfigurationTest.java @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.core.conf; + +import org.apache.accumulo.core.conf.AccumuloConfiguration.AllFilter; +import java.util.Map; +import org.junit.Before; +import org.junit.Test; +import static org.junit.Assert.assertEquals; + +public class DefaultConfigurationTest { + private DefaultConfiguration c; + + @Before + public void setUp() { + c = new DefaultConfiguration(); + } + + @Test + public void testGet() { + assertEquals(Property.MASTER_CLIENTPORT.getDefaultValue(), c.get(Property.MASTER_CLIENTPORT)); + } + + @Test + public void testGetProperties() { + Map<String,String> p = new java.util.HashMap<String,String>(); + c.getProperties(p, new AllFilter()); + assertEquals(Property.MASTER_CLIENTPORT.getDefaultValue(), p.get(Property.MASTER_CLIENTPORT.getKey())); + } +} http://git-wip-us.apache.org/repos/asf/accumulo/blob/37f900b7/docs/pom.xml ---------------------------------------------------------------------- diff --git a/docs/pom.xml b/docs/pom.xml index d54b144..a6260c6 100644 --- a/docs/pom.xml +++ b/docs/pom.xml @@ -62,7 +62,7 @@ </goals> <phase>compile</phase> <configuration> - <mainClass>org.apache.accumulo.core.conf.DefaultConfiguration</mainClass> + <mainClass>org.apache.accumulo.core.conf.ConfigurationDocGen</mainClass> <classpathScope>compile</classpathScope> <arguments> <argument>--generate-html</argument> @@ -77,7 +77,7 @@ </goals> <phase>compile</phase> <configuration> - <mainClass>org.apache.accumulo.core.conf.DefaultConfiguration</mainClass> + <mainClass>org.apache.accumulo.core.conf.ConfigurationDocGen</mainClass> <classpathScope>compile</classpathScope> <arguments> <argument>--generate-latex</argument>
