Return-Path: X-Original-To: apmail-accumulo-commits-archive@www.apache.org Delivered-To: apmail-accumulo-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 28D571103D for ; Wed, 13 Aug 2014 14:48:28 +0000 (UTC) Received: (qmail 21669 invoked by uid 500); 13 Aug 2014 14:48:28 -0000 Delivered-To: apmail-accumulo-commits-archive@accumulo.apache.org Received: (qmail 21590 invoked by uid 500); 13 Aug 2014 14:48:28 -0000 Mailing-List: contact commits-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list commits@accumulo.apache.org Received: (qmail 21453 invoked by uid 99); 13 Aug 2014 14:48:28 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 13 Aug 2014 14:48:27 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id 979209A51C4; Wed, 13 Aug 2014 14:48:27 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: bhavanki@apache.org To: commits@accumulo.apache.org Date: Wed, 13 Aug 2014 14:48:33 -0000 Message-Id: <1216ef2377a14171871331fd0d44751c@git.apache.org> In-Reply-To: <05606041fa644b80ab8781152699dd27@git.apache.org> References: <05606041fa644b80ab8781152699dd27@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [7/8] git commit: ACCUMULO-3012 Improve DefaultConfiguration 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 Authored: Thu Jul 24 09:36:04 2014 -0400 Committer: Bill Havanki 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 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> entries) { + String instanceZkTimeoutKey = Property.INSTANCE_ZK_TIMEOUT.getKey(); + String instanceZkTimeoutValue = null; + for (Entry 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 | --generate-latex "); + } + } + } 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 resolvedProps = null; - - synchronized public static DefaultConfiguration getInstance() { - if (instance == null) { - instance = new DefaultConfiguration(); - ConfigSanityCheck.validate(instance); + private final static Map resolvedProps; + static { + Map m = new HashMap(); + 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 getResolvedProps() { - if (resolvedProps == null) { - // the following loop is super slow, it takes a few milliseconds, so cache it - resolvedProps = new HashMap(); - 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 props, PropertyFilter filter) { - for (Entry entry : getResolvedProps().entrySet()) + for (Entry 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 | --generate-latex "); - } - } - } 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 m; + + @Before + public void setUp() { + m = new java.util.HashMap(); + } + + @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 p = new java.util.HashMap(); + 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 @@ compile - org.apache.accumulo.core.conf.DefaultConfiguration + org.apache.accumulo.core.conf.ConfigurationDocGen compile --generate-html @@ -77,7 +77,7 @@ compile - org.apache.accumulo.core.conf.DefaultConfiguration + org.apache.accumulo.core.conf.ConfigurationDocGen compile --generate-latex