ACCUMULO-3012 Improve DefaultConfiguration * The map of default properties created during the iterator() method each time is now static and immutable. * 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/7136996a Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/7136996a Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/7136996a Branch: refs/heads/master Commit: 7136996a19406a7dfc5c2f2c5861cae95f68badc Parents: c4d46bb 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 09:30:04 2014 -0400 ---------------------------------------------------------------------- .../accumulo/core/conf/ConfigSanityCheck.java | 44 +++++++++-- .../core/conf/DefaultConfiguration.java | 41 ++++++---- .../core/conf/ConfigSanityCheckTest.java | 79 ++++++++++++++++++++ .../core/conf/DefaultConfigurationTest.java | 47 ++++++++++++ 4 files changed, 187 insertions(+), 24 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/7136996a/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 6724670..abec58b 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 static 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/7136996a/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 d653274..c83c754 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 @@ -23,6 +23,8 @@ import java.io.PrintStream; import java.io.UnsupportedEncodingException; import java.util.ArrayList; import java.util.Iterator; +import java.util.Collections; +import java.util.Map; import java.util.Map.Entry; import java.util.TreeMap; @@ -30,32 +32,40 @@ import org.apache.accumulo.core.Constants; import org.apache.log4j.Logger; public class DefaultConfiguration extends AccumuloConfiguration { - private static DefaultConfiguration instance = null; private static Logger log = Logger.getLogger(DefaultConfiguration.class); - - 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 TreeMap<String,String>(); + for (Property prop : Property.values()) { + if (!prop.isExperimental() && !prop.getType().equals(PropertyType.PREFIX)) { + m.put(prop.getKey(), prop.getDefaultValue()); + } } - return instance; + ConfigSanityCheck.validate(m.entrySet()); + resolvedProps = Collections.unmodifiableMap(m); } - + + /** + * Gets a default configuration. + * + * @return default configuration + */ + public static DefaultConfiguration getInstance() { + return new DefaultConfiguration(); + } + @Override public String get(Property property) { return property.getDefaultValue(); } - + @Override public Iterator<Entry<String,String>> iterator() { - TreeMap<String,String> entries = new TreeMap<String,String>(); - for (Property prop : Property.values()) - if (!prop.isExperimental() && !prop.getType().equals(PropertyType.PREFIX)) - entries.put(prop.getKey(), prop.getDefaultValue()); - - return entries.entrySet().iterator(); + return resolvedProps.entrySet().iterator(); } + private static void generateDocumentation(PrintStream doc) { // read static content from resources and output InputStream data = DefaultConfiguration.class.getResourceAsStream("config.html"); @@ -185,5 +195,4 @@ public class DefaultConfiguration extends AccumuloConfiguration { throw new IllegalArgumentException("Usage: " + DefaultConfiguration.class.getName() + " --generate-doc <filename>"); } } - } http://git-wip-us.apache.org/repos/asf/accumulo/blob/7136996a/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/7136996a/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..9355fff --- /dev/null +++ b/core/src/test/java/org/apache/accumulo/core/conf/DefaultConfigurationTest.java @@ -0,0 +1,47 @@ +/* + * 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.Iterator; +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() { + Iterator<Map.Entry<String,String>> iter = c.iterator(); + while (iter.hasNext()) { + Map.Entry<String,String> e = iter.next(); + Property p = Property.getPropertyByKey(e.getKey()); + assertEquals(p.getDefaultValue(), e.getValue()); + } + } +}
