From a8edcf785e9194d0c70f819b72108f21ede52853 Mon Sep 17 00:00:00 2001 From: "Apekshit(Appy) Sharma" Date: Sun, 28 Jun 2015 18:22:47 -0700 Subject: [PATCH] HBASE-13957 Changes in this commit: - Adds ConfiguratinProperty class: Data object to contain all information about a property. - ConfigurationManager + Adds new interface to access values using ConfigurationProperty as param. get(ConfigurationParamenter conf) and set(ConfigurationParameter conf, value) + Deprecates observer related functions in lieu of oncoming changes to dynamic configuration framework. - Added 7 configurations to CommonConfigs, ServerConfigs and RegionServerConfigs depending on scope. Deprecated corresponding constants from HConstants (can not be deleted because audience is Public). - Use reflection in HBaseConfiguration to collect configurations and their default values. (Apekshit) --- .../org/apache/hadoop/hbase/MetaTableAccessor.java | 5 +- .../hbase/client/ConnectionImplementation.java | 5 +- .../org/apache/hadoop/hbase/CommonConfigs.java | 26 ++ .../hadoop/hbase/ConfigurationCollection.java | 34 +++ .../apache/hadoop/hbase/ConfigurationProperty.java | 190 ++++++++++++++ .../apache/hadoop/hbase/HBaseConfiguration.java | 33 +++ .../java/org/apache/hadoop/hbase/HConstants.java | 22 +- .../hadoop/hbase/TestConfigurationProperty.java | 192 ++++++++++++++ .../org/apache/hadoop/hbase/HealthCheckChore.java | 15 +- .../org/apache/hadoop/hbase/ServerConfigs.java | 31 +++ .../hadoop/hbase/conf/ConfigurationManager.java | 192 +++++++++++++- .../org/apache/hadoop/hbase/master/HMaster.java | 5 +- .../hadoop/hbase/regionserver/HRegionServer.java | 30 ++- .../hbase/regionserver/RegionServerConfigs.java | 37 +++ .../org/apache/hadoop/hbase/util/HBaseFsck.java | 3 +- .../hadoop/hbase/TestNodeHealthCheckChore.java | 48 ++-- .../hadoop/hbase/client/TestMetaWithReplicas.java | 8 +- .../hbase/conf/TestConfigurationManager.java | 285 ++++++++++++++++++++- pom.xml | 18 ++ 19 files changed, 1113 insertions(+), 66 deletions(-) create mode 100644 hbase-common/src/main/java/org/apache/hadoop/hbase/CommonConfigs.java create mode 100644 hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationCollection.java create mode 100644 hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationProperty.java create mode 100644 hbase-common/src/test/java/org/apache/hadoop/hbase/TestConfigurationProperty.java create mode 100644 hbase-server/src/main/java/org/apache/hadoop/hbase/ServerConfigs.java create mode 100644 hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerConfigs.java diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java index 2fbfd9f..dbb3521 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/MetaTableAccessor.java @@ -551,8 +551,9 @@ public class MetaTableAccessor { int scannerCaching = connection.getConfiguration() .getInt(HConstants.HBASE_META_SCANNER_CACHING, HConstants.DEFAULT_HBASE_META_SCANNER_CACHING); - if (connection.getConfiguration().getBoolean(HConstants.USE_META_REPLICAS, - HConstants.DEFAULT_USE_META_REPLICAS)) { + if (connection.getConfiguration().getBoolean( + CommonConfigs.HBASE_META_REPLICAS_USE.getName(), + CommonConfigs.HBASE_META_REPLICAS_USE.getDefaultValue())) { scan.setConsistency(Consistency.TIMELINE); } if (rowUpperLimit <= scannerCaching) { diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java index 35ff34f..310cf2b 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java @@ -26,6 +26,7 @@ import com.google.protobuf.ServiceException; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.CommonConfigs; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.HRegionInfo; @@ -220,8 +221,8 @@ class ConnectionImplementation implements ClusterConnection, Closeable { this.closed = false; this.pause = conf.getLong(HConstants.HBASE_CLIENT_PAUSE, HConstants.DEFAULT_HBASE_CLIENT_PAUSE); - this.useMetaReplicas = conf.getBoolean(HConstants.USE_META_REPLICAS, - HConstants.DEFAULT_USE_META_REPLICAS); + this.useMetaReplicas = conf.getBoolean(CommonConfigs.HBASE_META_REPLICAS_USE.getName(), + CommonConfigs.HBASE_META_REPLICAS_USE.getDefaultValue()); this.numTries = tableConfig.getRetriesNumber(); this.rpcTimeout = conf.getInt( HConstants.HBASE_RPC_TIMEOUT_KEY, diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/CommonConfigs.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/CommonConfigs.java new file mode 100644 index 0000000..9179d2b --- /dev/null +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/CommonConfigs.java @@ -0,0 +1,26 @@ +/** + * 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.hadoop.hbase; + +@ConfigurationCollection +public class CommonConfigs { + public static final ConfigurationProperty HBASE_META_REPLICAS_USE = + new ConfigurationProperty.Builder<>( + HConstants.USE_META_REPLICAS, HConstants.DEFAULT_USE_META_REPLICAS) + .setDescription("If true, meta replication is enabled.").build(); +} diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationCollection.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationCollection.java new file mode 100644 index 0000000..cf94577 --- /dev/null +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationCollection.java @@ -0,0 +1,34 @@ +/** + * 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.hadoop.hbase; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Annotation for classes containing {@link ConfigurationProperty}. + * {@link HBaseConfiguration} uses {@link org.reflections.Reflections} to discover classes + * annotated with this annotation and adds the properties defined in them to its own + * {@link org.apache.hadoop.conf.Configuration} object. + */ +@Retention(RetentionPolicy.RUNTIME) +@Target(ElementType.TYPE) +public @interface ConfigurationCollection { +} diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationProperty.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationProperty.java new file mode 100644 index 0000000..31af1db --- /dev/null +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/ConfigurationProperty.java @@ -0,0 +1,190 @@ +/** + * 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.hadoop.hbase; + +import java.lang.reflect.Array; + +/** + * DataObject for configuration properties. + * Stores all information related to a property like name, default value, description, unit of + * value, etc. + * A property with {@code inRefGuide} set to true should have a non-empty description, otherwise + * {@link IllegalArgumentException} is thrown from the constructor. + * + * @param Type of property. Accepted types are String, Boolean, Integer, Long, Float, Double, + * String[]. + */ +public class ConfigurationProperty { + private String name; + private T defaultValue; + private String description; + private Unit unit; + /** + * If true, property is listed in the HBase reference guide. + * Description for such a property can not be empty, else IllegalArgumentException is thrown. + */ + private boolean inRefGuide; + private boolean isDynamic; + + public static class Builder { + // required + private String name; + private K defaultValue; + // optional + private String description = ""; + private Unit unit = Unit.NONE; + private boolean inRefGuide = false; + private boolean isDynamic = false; + + public Builder(String name, K defaultValue) { + this.name = name; + this.defaultValue = defaultValue; + } + + public Builder setDescription(String description) { + this.description = description; + return this; + } + + public Builder setUnit(Unit unit) { + this.unit = unit; + return this; + } + + public Builder setInRefGuide() { + this.inRefGuide = true; + return this; + } + + public Builder setIsDynamic() { + this.isDynamic = true; + return this; + } + + public ConfigurationProperty build() { + return new ConfigurationProperty(this); + } + } + + private ConfigurationProperty(Builder builder) { + name = builder.name; + defaultValue = builder.defaultValue; + description = builder.description; + unit = builder.unit; + inRefGuide = builder.inRefGuide; + isDynamic = builder.isDynamic; + if (inRefGuide && description.isEmpty()) { + throw new IllegalArgumentException("Description can not be empty for a property referenced " + + "in guide. property = " + name); + } + } + + public ConfigurationProperty(String name, T defaultValue) { + this(new Builder(name, defaultValue)); + } + + public String getName() { + return name; + } + + public T getDefaultValue() { + return defaultValue; + } + + public String getValueType() { + return defaultValue.getClass().getSimpleName(); + } + + public String getDescription() { + return description; + } + + public Unit getUnit() { + return unit; + } + + public boolean inRefGuide() { + return inRefGuide; + } + + public boolean isDynamic() { + return isDynamic; + } + + public enum Unit { + NONE("none"), + + // Units of time. + MILLISECONDS("millis"), + MICROSECONDS("micros"), + SECONDS("seconds"), + MINUTES("minutes"), + HOURS("hours"), + DAYS("days"), + + // Units of bytes + BYTES("bytes"), + KILOBYTES("KiB"), + MEGABYTES("MiB"), + GIGABYTES("GiB"), + + // other + PERCENT("percent"), + FRACTION("fraction of one"), + ; + + String label; + + public String getLabel() { + return label; + } + + Unit(String label) { + this.label = label; + } + } + + @Override + /** + * @return string representation of default value which can be re-parsed to its corresponding + * type using {@link org.apache.hadoop.conf.Configuration} class. + * @throws IllegalArgumentException on encountering a property with unexpected type. + */ + public String toString() { + Class valueClass = defaultValue.getClass(); + // isPrimitive() checks for Boolean, Long, Integer, Float, Double, etc + if (defaultValue instanceof String || defaultValue instanceof Boolean + || defaultValue instanceof Integer || defaultValue instanceof Long + || defaultValue instanceof Float || defaultValue instanceof Double) { + return defaultValue.toString(); + } else if (valueClass.isArray() && valueClass.getComponentType().equals(String.class)) { + // if the default value is an array of String, iterate over array items using reflection + // and build a String of comma separated values. + StringBuilder stringValueBuilder = new StringBuilder(); + int length = Array.getLength(defaultValue); + for (int i = 0; i < length; ++i) { + stringValueBuilder.append(Array.get(defaultValue, i).toString()).append(","); + } + String stringValue = stringValueBuilder.toString(); + return stringValue.substring(0, stringValue.length() - 1); + } else { + throw new IllegalArgumentException("Unexpected value type '" + valueClass.getName() + + "' for ConfigurationProperty " + getName()); + } + } +} diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java index 1be66a1..2014db3 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java @@ -18,9 +18,14 @@ package org.apache.hadoop.hbase; import java.io.IOException; +import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.HashMap; +import java.util.Map; import java.util.Map.Entry; +import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -29,6 +34,7 @@ import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.hbase.classification.InterfaceStability; import org.apache.hadoop.hbase.io.util.HeapMemorySizeUtil; import org.apache.hadoop.hbase.util.VersionInfo; +import org.reflections.Reflections; /** * Adds HBase configuration files to a Configuration @@ -76,8 +82,35 @@ public class HBaseConfiguration extends Configuration { } } + /** + * Adds configurations (static fields of type {@link ConfigurationProperty}) from classes + * annotated with {@link ConfigurationCollection} to {@code conf}. + */ + // TODO(apekshit): time this function for overheads. + private static Configuration addConfigurationCollections(Configuration conf) { + Reflections reflections = Reflections.collect(); + Set> configCollections = reflections.getTypesAnnotatedWith( + ConfigurationCollection.class); + for (Class configCollection : configCollections) { + for (Field field : configCollection.getDeclaredFields()) { + if (!field.getDeclaringClass().equals(ConfigurationProperty.class)) { + continue; + } + ConfigurationProperty property = null; + try { + property = (ConfigurationProperty) field.get(null); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + conf.set(property.getName(), property.toString(), configCollection.getSimpleName()); + } + } + return conf; + } + public static Configuration addHbaseResources(Configuration conf) { conf.addResource("hbase-default.xml"); + addConfigurationCollections(conf); conf.addResource("hbase-site.xml"); checkDefaultsVersion(conf); diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java index 32f07cb..81c3647 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java @@ -931,7 +931,11 @@ public final class HConstants { "hbase.master.log.replay.wait.region.timeout"; /** Conf key for enabling meta replication */ + /** Use {@link CommonConfigs#HBASE_META_REPLICAS_USE} instead. */ + @Deprecated public static final String USE_META_REPLICAS = "hbase.meta.replicas.use"; + /** Use {@link CommonConfigs#HBASE_META_REPLICAS_USE} instead. */ + @Deprecated public static final boolean DEFAULT_USE_META_REPLICAS = false; public static final String META_REPLICAS_NUM = "hbase.meta.replica.count"; public static final int DEFAULT_META_REPLICA_NUM = 1; @@ -1029,17 +1033,27 @@ public final class HConstants { new String[] { TableName.META_TABLE_NAME.getNameAsString() }, HBASE_NON_TABLE_DIRS.toArray()))); - /** Health script related settings. */ + // Health script related settings. + /** Use {@link ServerConfigs#HBASE_NODE_HEALTH_SCRIPT_LOCATION} instead. */ + @Deprecated public static final String HEALTH_SCRIPT_LOC = "hbase.node.health.script.location"; + /** Use {@link ServerConfigs#HBASE_NODE_HEALTH_SCRIPT_TIMEOUT} instead. */ + @Deprecated public static final String HEALTH_SCRIPT_TIMEOUT = "hbase.node.health.script.timeout"; + /** Use {@link ServerConfigs#HBASE_NODE_HEALTH_SCRIPT_FREQUENCY} instead. */ + @Deprecated public static final String HEALTH_CHORE_WAKE_FREQ = "hbase.node.health.script.frequency"; + /** Use {@link ServerConfigs#HBASE_NODE_HEALTH_SCRIPT_TIMEOUT} instead. */ + @Deprecated public static final long DEFAULT_HEALTH_SCRIPT_TIMEOUT = 60000; - /** - * The maximum number of health check failures a server can encounter consecutively. - */ + /** The maximum number of health check failures a server can encounter consecutively. */ + /** Use {@link ServerConfigs#HBASE_NODE_HEALTH_FAILURE_THRESHOLD} instead. */ + @Deprecated public static final String HEALTH_FAILURE_THRESHOLD = "hbase.node.health.failure.threshold"; + /** Use {@link ServerConfigs#HBASE_NODE_HEALTH_FAILURE_THRESHOLD} instead. */ + @Deprecated public static final int DEFAULT_HEALTH_FAILURE_THRESHOLD = 3; diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/TestConfigurationProperty.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/TestConfigurationProperty.java new file mode 100644 index 0000000..06a170e --- /dev/null +++ b/hbase-common/src/test/java/org/apache/hadoop/hbase/TestConfigurationProperty.java @@ -0,0 +1,192 @@ +/** + * 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.hadoop.hbase; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.reflections.Reflections; +import org.apache.hadoop.hbase.testclassification.SmallTests; + +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +@Category({SmallTests.class}) +public class TestConfigurationProperty { + private static final Log LOG = LogFactory.getLog(TestConfigurationProperty.class); + + Reflections reflections; + Set> configCollections; + + @Before + public void setup() { + reflections = Reflections.collect(); + configCollections = reflections.getTypesAnnotatedWith(ConfigurationCollection.class); + } + + @Test + /** + * Tests {@link ConfigurationProperty#toString()}. + */ + public void testValueToString() { + ConfigurationProperty strProperty = + new ConfigurationProperty<>("string.property", "foo"); + assertEquals(strProperty.toString(), "foo"); + + ConfigurationProperty boolProperty = + new ConfigurationProperty<>("boolean.property", false); + assertEquals(boolProperty.toString(), "false"); + + ConfigurationProperty intProperty = + new ConfigurationProperty<>("int.property", 1); + assertEquals(intProperty.toString(), "1"); + + ConfigurationProperty longProperty = + new ConfigurationProperty<>("long.property", 999999999L); + assertEquals(longProperty.toString(), "999999999"); + + ConfigurationProperty floatProperty = + new ConfigurationProperty<>("float.property", 1.0f); + assertEquals(floatProperty.toString(), "1.0"); + + ConfigurationProperty doubleProperty = + new ConfigurationProperty<>("double.property", 2.0); + assertEquals(doubleProperty.toString(), "2.0"); + + ConfigurationProperty strArrayProperty = + new ConfigurationProperty<>("str.array.property", new String[]{"foo", "bar"}); + assertEquals(strArrayProperty.toString(), "foo,bar"); + } + + // TODO: @reviewers, what would be right place to put these tests? + @Test + public void testConfigurationPropertyFieldsAreStatic() { + boolean success = true; + for (Class configCollection : configCollections) { + for (Field field : configCollection.getDeclaredFields()) { + if (!Modifier.isStatic(field.getModifiers())) { + LOG.error("ConfigurationProperty '" + field.getName() + "' in class '" + + configCollections.getClass().getName() + "' is not static."); + success = false; + } + } + } + assertTrue(success); + } + + @Test + /** + * Tests that names of ConfigurationProperty fields follow the convention i.e. + * 'foo.bar' -> FOO_BAR + */ + public void testConfigurationPropertyFieldsFollowNamingConvention() { + boolean success = true; + for (Class configCollection : configCollections) { + for (Field field : configCollection.getDeclaredFields()) { + ConfigurationProperty property = null; + try { + property = (ConfigurationProperty) field.get(null); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + String expectedName = property.getName().toUpperCase().replace('.', '_'); + if (!field.getName().equals(expectedName)) { + LOG.error("Bad variable name '" + field.getName() + "' for" + " property '" + property.getName() + + "'. Should be " + expectedName); + success = false; + } + } + } + assertTrue(success); + } + + @Test + /** + * Tests that only one ConfigurationProperty field is defined for each property. + */ + public void testConfigurationPropertyDuplicates() { + boolean success = true; + Map properties = new HashMap<>(); + for (Class configCollection : configCollections) { + for (Field field : configCollection.getDeclaredFields()) { + ConfigurationProperty property = null; + try { + property = (ConfigurationProperty) field.get(null); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + if (properties.containsKey(property.getName())) { + LOG.error("Duplicate definition of ConfigurationProperty for '" + property.getName() + + "'. Defined in classes " + configCollection.getName() + " and " + + properties.get(property.getName())); + success = false; + } else { + properties.put(property.getName(), configCollection.getName()); + } + } + } + assertTrue(success); + } + + @Test + /** + * To promote the clean design of classes, components and packages grouping their configurations + * together in a separate class or sub-class. These classes should only have + * ConfigurationProperty fields to prevent unexpected designs creeping in. + */ + // Note to reviewers: Seems good to me to impose this restriction. If there is a good reason, + // we can totally remove this. + public void testConfigurationCollectionClassesAreClean() { + boolean success = true; + for (Class configCollection : configCollections) { + String className = configCollection.getName(); + Class[] classes = configCollection.getClasses(); + if (classes.length != 0) { + success = false; + LOG.error("ConfigurationCollection class '" + className + "' has unwanted classes: " + + Arrays.deepToString(classes)); + } + // getMethods() can not be used here because it'll return functions from {@link Object} class. + Method[] methods = configCollection.getDeclaredMethods(); + if (methods.length != 0) { + success = false; + LOG.error("ConfigurationCollection class '" + className + "' has unwanted methods: " + + Arrays.deepToString(methods)); + } + for (Field field : configCollection.getDeclaredFields()) { + if (!field.getType().equals(ConfigurationProperty.class)) { + success = false; + LOG.error("ConfigurationCollection class '" + className + "' has field '" + + field.getName() + "' of unexpected type " + field.getType()); + } + } + } + assertTrue(success); + } +} + diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/HealthCheckChore.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/HealthCheckChore.java index ff9f94b..84d8abb 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/HealthCheckChore.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/HealthCheckChore.java @@ -21,8 +21,8 @@ package org.apache.hadoop.hbase; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HealthChecker.HealthCheckerExitStatus; +import org.apache.hadoop.hbase.conf.ConfigurationManager; import org.apache.hadoop.util.StringUtils; /** @@ -31,23 +31,20 @@ import org.apache.hadoop.util.StringUtils; public class HealthCheckChore extends ScheduledChore { private static final Log LOG = LogFactory.getLog(HealthCheckChore.class); private HealthChecker healthChecker; - private Configuration config; private int threshold; private int numTimesUnhealthy = 0; private long failureWindow; private long startWindow; - public HealthCheckChore(int sleepTime, Stoppable stopper, Configuration conf) { + public HealthCheckChore(int sleepTime, Stoppable stopper, ConfigurationManager confManager) { super("HealthChecker", stopper, sleepTime); LOG.info("Health Check Chore runs every " + StringUtils.formatTime(sleepTime)); - this.config = conf; - String healthCheckScript = this.config.get(HConstants.HEALTH_SCRIPT_LOC); - long scriptTimeout = this.config.getLong(HConstants.HEALTH_SCRIPT_TIMEOUT, - HConstants.DEFAULT_HEALTH_SCRIPT_TIMEOUT); + String healthCheckScript = confManager.getString( + ServerConfigs.HBASE_NODE_HEALTH_SCRIPT_LOCATION); + long scriptTimeout = confManager.getLong(ServerConfigs.HBASE_NODE_HEALTH_SCRIPT_TIMEOUT); healthChecker = new HealthChecker(); healthChecker.init(healthCheckScript, scriptTimeout); - this.threshold = config.getInt(HConstants.HEALTH_FAILURE_THRESHOLD, - HConstants.DEFAULT_HEALTH_FAILURE_THRESHOLD); + this.threshold = confManager.getInt(ServerConfigs.HBASE_NODE_HEALTH_FAILURE_THRESHOLD); this.failureWindow = (long)this.threshold * (long)sleepTime; } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/ServerConfigs.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/ServerConfigs.java new file mode 100644 index 0000000..2cb0b3a --- /dev/null +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/ServerConfigs.java @@ -0,0 +1,31 @@ +/** + * 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.hadoop.hbase; + +@ConfigurationCollection +public class ServerConfigs { + public static final ConfigurationProperty HBASE_NODE_HEALTH_FAILURE_THRESHOLD = + new ConfigurationProperty<>(HConstants.HEALTH_FAILURE_THRESHOLD, + HConstants.DEFAULT_HEALTH_FAILURE_THRESHOLD); + public static final ConfigurationProperty HBASE_NODE_HEALTH_SCRIPT_LOCATION = + new ConfigurationProperty<>(HConstants.HEALTH_SCRIPT_LOC, ""); + public static final ConfigurationProperty HBASE_NODE_HEALTH_SCRIPT_TIMEOUT = + new ConfigurationProperty.Builder<>( + HConstants.HEALTH_SCRIPT_TIMEOUT, HConstants.DEFAULT_HEALTH_SCRIPT_TIMEOUT) + .setUnit(ConfigurationProperty.Unit.MILLISECONDS).build(); +} diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/conf/ConfigurationManager.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/conf/ConfigurationManager.java index 29b3e8b..ce3c5dc 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/conf/ConfigurationManager.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/conf/ConfigurationManager.java @@ -20,18 +20,26 @@ package org.apache.hadoop.hbase.conf; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.ConfigurationProperty; import org.apache.hadoop.hbase.classification.InterfaceAudience; import org.apache.hadoop.hbase.classification.InterfaceStability; +import org.apache.hadoop.hbase.util.Classes; +import javax.xml.bind.TypeConstraintException; import java.util.Collections; import java.util.Set; import java.util.WeakHashMap; /** - * Maintains the set of all the classes which would like to get notified - * when the Configuration is reloaded from the disk in the Online Configuration - * Change mechanism, which lets you update certain configuration properties - * on-the-fly, without having to restart the cluster. + * ConfigurationManager manages: + * 1) underlying configuration and provides access to properties' values. Moving forward, all + * internal HBase code should move from using Configuration to ConfigurationManager. HBASE-13936 + * 2) dynamic configuration updates: Maintains a set of all the classes which would like + * to get notified when the Configuration is reloaded from the disk in the Online + * Configuration Change mechanism, which lets you update certain configuration + * properties on-the-fly, without having to restart the cluster. + * Note: Functions related to observers have been deprecated in lieu of oncoming changes to + * dynamic configuration framework. HBASE-13936 * * If a class has configuration properties which you would like to be able to * change on-the-fly, do the following: @@ -70,23 +78,193 @@ import java.util.WeakHashMap; * practice to deregister your observer, whenever possible. * */ +// TODO(apekshit) : move to hbase-common so client can access it too. @InterfaceAudience.Private @InterfaceStability.Evolving public class ConfigurationManager { private static final Log LOG = LogFactory.getLog(ConfigurationManager.class); + public ConfigurationManager(Configuration configuration) { + this.configuration = configuration; + } + + private Configuration configuration; + + // Getters + + /** + * @return expanded string value of the {@code property}. If the property is deprecated, it + * returns the value of the first property which replaces the deprecated property and is not null. + * Values are processed for variable expansion before being + * returned. + */ + public String getString(ConfigurationProperty property) { + return configuration.get(property.getName()); + } + + /** + * @return value of the given {@code property} as boolean. + * @throws IllegalArgumentException if value is invalid i.e. neither 'true' or 'false' + * (case-insensitive). + */ + public boolean getBoolean(ConfigurationProperty property) { + String stringValue = configuration.get(property.getName()); + if (stringValue == null) { + return property.getDefaultValue(); + } + if ("true".equalsIgnoreCase(stringValue)) { + return true; + } else if ("false".equalsIgnoreCase(stringValue)) { + return false; + } else throw new IllegalArgumentException( + "Configuration Error: Boolean configuration " + property.getName() + + " set to bad value '" + stringValue + "'. Using default value " + + property.getDefaultValue()); + } + + /** + * @return value of the given {@code property} as an int. + * @throws NumberFormatException if the value can not be parsed as an int. + */ + public int getInt(ConfigurationProperty property) { + return configuration.getInt(property.getName(), property.getDefaultValue()); + } + + /** + * @return value of the given {@code property} as a long. + * @throws NumberFormatException if the value can not be parsed as a long. + */ + public long getLong(ConfigurationProperty property) { + return configuration.getLong(property.getName(), property.getDefaultValue()); + } + + /** + * @return value of the given {@code property} as a float. + * @throws NumberFormatException if the value can not be parsed as a float. + */ + public float getFloat(ConfigurationProperty property) { + return configuration.getFloat(property.getName(), property.getDefaultValue()); + } + + /** + * @return value of the given {@code property} as a double. + * @throws NumberFormatException if the value can not be parsed as a double. + */ + public double getDouble(ConfigurationProperty property) { + return configuration.getDouble(property.getName(), property.getDefaultValue()); + } + + /** + * @return value of the {@code property} as a {@code>String[]} after splitting using + * comma as the delimiter. If the value is an empty string, {@code null} is returned. + */ + public String[] getStrings(ConfigurationProperty property) { + return configuration.getStrings(property.getName(), property.getDefaultValue()); + } + + /** + * @return value of the given {@code property} as a {@code Class}. + * @throws TypeNotPresentException if the class is not found. + */ + public Class getClass(ConfigurationProperty> property) { + String className = configuration.get(property.getName()); + if (className.isEmpty()) { + return property.getDefaultValue(); + } + try { + return configuration.getClassByName(className); + } catch (ClassNotFoundException e) { + throw new TypeNotPresentException(className, e); + } + } + + /** + * @param baseClass class specified as the value of the {@code property} should be + * sub-class of this class. + * @return value of the given {@code property} as a {@code Class}. + * @throws TypeNotPresentException if the class is not found or does not implements + * {@code baseClass}. + */ + public Class getClass(ConfigurationProperty> property, + Class baseClass) throws ClassNotFoundException { + Class theClass = getClass((ConfigurationProperty) property); + try { + return theClass.asSubclass(baseClass); + } catch (ClassCastException e) { + throw new TypeNotPresentException(theClass.getName(), new Throwable(theClass + " not " + + baseClass)); + } + } + + // Setters + + /** + * Set the value of the {@code property}. If property is deprecated, it also + * sets the keys that replace the deprecated key to the {@code value}. + * + * @throws IllegalArgumentException if the value is null. + */ + public void setString(ConfigurationProperty property, String value) { + configuration.set(property.getName(), value, null); + } + + public void setBoolean(ConfigurationProperty property, boolean value) { + configuration.setBoolean(property.getName(), value); + } + + public void setInt(ConfigurationProperty property, int value) { + configuration.setInt(property.getName(), value); + } + + public void setLong(ConfigurationProperty property, long value) { + configuration.setLong(property.getName(), value); + } + + public void setFloat(ConfigurationProperty property, float value) { + configuration.setFloat(property.getName(), value); + } + + public void setDouble(ConfigurationProperty property, double value) { + configuration.setDouble(property.getName(), value); + } + + /** + * Set the value of the {@code property} to comma separated string {@code value}s. + */ + public void setStrings(ConfigurationProperty property, String... value) { + configuration.setStrings(property.getName(), value); + } + + public void setClass(ConfigurationProperty> property, Class value) { + configuration.set(property.getName(), value.getName()); + } + + /** + * Sets the {@code property} to name of the {@code value} class. + * @param baseClass {@code value} should be a sub-class of this class. + */ + public void setClass(ConfigurationProperty> property, + Class value, Class baseClass) { + configuration.setClass(property.getName(), value, baseClass); + } + + public Configuration getConfiguration() { + return configuration; + } + // The set of Configuration Observers. These classes would like to get // notified when the configuration is reloaded from disk. This is a set // constructed from a WeakHashMap, whose entries would be removed if the // observer classes go out of scope. + @Deprecated private Set configurationObservers = Collections.newSetFromMap(new WeakHashMap()); /** * Register an observer class - * @param observer */ + @Deprecated public void registerObserver(ConfigurationObserver observer) { synchronized (configurationObservers) { configurationObservers.add(observer); @@ -98,8 +276,8 @@ public class ConfigurationManager { /** * Deregister an observer class - * @param observer */ + @Deprecated public void deregisterObserver(ConfigurationObserver observer) { synchronized (configurationObservers) { configurationObservers.remove(observer); @@ -113,6 +291,7 @@ public class ConfigurationManager { * The conf object has been repopulated from disk, and we have to notify * all the observers that are expressed interest to do that. */ + @Deprecated public void notifyAllObservers(Configuration conf) { synchronized (configurationObservers) { for (ConfigurationObserver observer : configurationObservers) { @@ -131,6 +310,7 @@ public class ConfigurationManager { /** * @return the number of observers. */ + @Deprecated public int getNumObservers() { synchronized (configurationObservers) { return configurationObservers.size(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index ff5cb64..786cb39 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -49,6 +49,7 @@ import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.ClusterStatus; +import org.apache.hadoop.hbase.CommonConfigs; import org.apache.hadoop.hbase.CoordinatedStateException; import org.apache.hadoop.hbase.CoordinatedStateManager; import org.apache.hadoop.hbase.DoNotRetryIOException; @@ -356,7 +357,7 @@ public class HMaster extends HRegionServer implements MasterServices, Server { ", hbase.cluster.distributed=" + this.conf.getBoolean(HConstants.CLUSTER_DISTRIBUTED, false)); // Disable usage of meta replicas in the master - this.conf.setBoolean(HConstants.USE_META_REPLICAS, false); + this.conf.setBoolean(CommonConfigs.HBASE_META_REPLICAS_USE.getName(), false); Replication.decorateMasterConfiguration(this.conf); @@ -769,7 +770,7 @@ public class HMaster extends HRegionServer implements MasterServices, Server { status.markComplete("Initialization successful"); LOG.info("Master has completed initialization"); - configurationManager.registerObserver(this.balancer); + confManager.registerObserver(this.balancer); // Set master as 'initialized'. initialized = true; // assign the meta replicas diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index 43c2836..ee33f52 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -60,6 +60,7 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.ChoreService; import org.apache.hadoop.hbase.ClockOutOfSyncException; +import org.apache.hadoop.hbase.CommonConfigs; import org.apache.hadoop.hbase.CoordinatedStateManager; import org.apache.hadoop.hbase.CoordinatedStateManagerFactory; import org.apache.hadoop.hbase.HBaseConfiguration; @@ -70,6 +71,7 @@ import org.apache.hadoop.hbase.HealthCheckChore; import org.apache.hadoop.hbase.MetaTableAccessor; import org.apache.hadoop.hbase.NotServingRegionException; import org.apache.hadoop.hbase.ScheduledChore; +import org.apache.hadoop.hbase.ServerConfigs; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.Stoppable; import org.apache.hadoop.hbase.TableDescriptors; @@ -477,7 +479,7 @@ public class HRegionServer extends HasThread implements * Configuration manager is used to register/deregister and notify the configuration observers * when the regionserver is notified that there was a change in the on disk configs. */ - protected final ConfigurationManager configurationManager; + protected final ConfigurationManager confManager; /** * Starts a HRegionServer at the default location. @@ -505,7 +507,7 @@ public class HRegionServer extends HasThread implements Superusers.initialize(conf); FSUtils.setupShortCircuitRead(this.conf); // Disable usage of meta replicas in the regionserver - this.conf.setBoolean(HConstants.USE_META_REPLICAS, false); + this.conf.setBoolean(CommonConfigs.HBASE_META_REPLICAS_USE.getName(), false); // Config'ed params this.numRetries = this.conf.getInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, @@ -593,7 +595,7 @@ public class HRegionServer extends HasThread implements clusterStatusTracker = new ClusterStatusTracker(zooKeeper, this); clusterStatusTracker.start(); } - this.configurationManager = new ConfigurationManager(); + this.confManager = new ConfigurationManager(conf); rpcServices.start(); putUpWebUI(); @@ -727,9 +729,8 @@ public class HRegionServer extends HasThread implements // Health checker thread. if (isHealthCheckerConfigured()) { - int sleepTime = this.conf.getInt(HConstants.HEALTH_CHORE_WAKE_FREQ, - HConstants.DEFAULT_THREAD_WAKE_FREQUENCY); - healthCheckChore = new HealthCheckChore(sleepTime, this, getConfiguration()); + int sleepTime = confManager.getInt(RegionServerConfigs.HBASE_NODE_HEALTH_SCRIPT_FREQUENCY); + healthCheckChore = new HealthCheckChore(sleepTime, this, getConfigurationManager()); } this.pauseMonitor = new JvmPauseMonitor(conf); pauseMonitor.start(); @@ -868,7 +869,7 @@ public class HRegionServer extends HasThread implements private void registerConfigurationObservers() { // Registering the compactSplitThread object with the ConfigurationManager. - configurationManager.registerObserver(this.compactSplitThread); + confManager.registerObserver(this.compactSplitThread); } /** @@ -1738,9 +1739,11 @@ public class HRegionServer extends HasThread implements // tasks even after current task is preempted after a split task times out. Configuration sinkConf = HBaseConfiguration.create(conf); sinkConf.setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, - conf.getInt("hbase.log.replay.retries.number", 8)); // 8 retries take about 23 seconds + conf.getInt(RegionServerConfigs.HBASE_LOG_REPLAY_RETRIES_NUMBER.getName(), + RegionServerConfigs.HBASE_LOG_REPLAY_RETRIES_NUMBER.getDefaultValue())); sinkConf.setInt(HConstants.HBASE_RPC_TIMEOUT_KEY, - conf.getInt("hbase.log.replay.rpc.timeout", 30000)); // default 30 seconds + conf.getInt(RegionServerConfigs.HBASE_LOG_REPLAY_RPC_TIMEOUT.getName(), + RegionServerConfigs.HBASE_LOG_REPLAY_RPC_TIMEOUT.getDefaultValue())); sinkConf.setInt("hbase.client.serverside.retries.multiplier", 1); this.splitLogWorker = new SplitLogWorker(this, sinkConf, this, this, walFactory); splitLogWorker.start(); @@ -2446,7 +2449,7 @@ public class HRegionServer extends HasThread implements @Override public void addToOnlineRegions(Region region) { this.onlineRegions.put(region.getRegionInfo().getEncodedName(), region); - configurationManager.registerObserver(region); + confManager.registerObserver(region); } /** @@ -3128,7 +3131,8 @@ public class HRegionServer extends HasThread implements } private boolean isHealthCheckerConfigured() { - String healthScriptLocation = this.conf.get(HConstants.HEALTH_SCRIPT_LOC); + String healthScriptLocation = + confManager.getString(ServerConfigs.HBASE_NODE_HEALTH_SCRIPT_LOCATION); return org.apache.commons.lang.StringUtils.isNotBlank(healthScriptLocation); } @@ -3284,7 +3288,7 @@ public class HRegionServer extends HasThread implements * @return : Returns the ConfigurationManager object for testing purposes. */ protected ConfigurationManager getConfigurationManager() { - return configurationManager; + return confManager; } /** @@ -3301,7 +3305,7 @@ public class HRegionServer extends HasThread implements LOG.info("Reloading the configuration from disk."); // Reload the configuration from disk. conf.reloadConfiguration(); - configurationManager.notifyAllObservers(conf); + confManager.notifyAllObservers(conf); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerConfigs.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerConfigs.java new file mode 100644 index 0000000..4d0e11f --- /dev/null +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerConfigs.java @@ -0,0 +1,37 @@ +/** + * 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.hadoop.hbase.regionserver; + +import org.apache.hadoop.hbase.ConfigurationCollection; +import org.apache.hadoop.hbase.ConfigurationProperty; +import org.apache.hadoop.hbase.HConstants; + +@ConfigurationCollection +public class RegionServerConfigs { + // 8 retries take about 23 seconds. + public static final ConfigurationProperty HBASE_LOG_REPLAY_RETRIES_NUMBER = + new ConfigurationProperty<>("hbase.log.replay.retries.number", 8); + + public static final ConfigurationProperty HBASE_LOG_REPLAY_RPC_TIMEOUT = + new ConfigurationProperty.Builder<>("hbase.log.replay.rpc.timeout", 30000) + .setUnit(ConfigurationProperty.Unit.MILLISECONDS).build(); + + public static final ConfigurationProperty HBASE_NODE_HEALTH_SCRIPT_FREQUENCY = + new ConfigurationProperty<>(HConstants.HEALTH_CHORE_WAKE_FREQ, + HConstants.DEFAULT_THREAD_WAKE_FREQUENCY); +} diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java index cc87f64..ad1bb7b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java @@ -78,6 +78,7 @@ import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hbase.Abortable; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.ClusterStatus; +import org.apache.hadoop.hbase.CommonConfigs; import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.HColumnDescriptor; @@ -317,7 +318,7 @@ public class HBaseFsck extends Configured implements Closeable { // disable blockcache for tool invocation, see HBASE-10500 getConf().setFloat(HConstants.HFILE_BLOCK_CACHE_SIZE_KEY, 0); // Disable usage of meta replicas in hbck - getConf().setBoolean(HConstants.USE_META_REPLICAS, false); + getConf().setBoolean(CommonConfigs.HBASE_META_REPLICAS_USE.getName(), false); errors = getErrorReporter(conf); int numThreads = conf.getInt("hbasefsck.numthreads", MAX_NUM_THREADS); diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestNodeHealthCheckChore.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestNodeHealthCheckChore.java index 9360b1f..2683014 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/TestNodeHealthCheckChore.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/TestNodeHealthCheckChore.java @@ -29,14 +29,15 @@ import java.util.UUID; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.hbase.HealthChecker.HealthCheckerExitStatus; +import org.apache.hadoop.hbase.conf.ConfigurationManager; import org.apache.hadoop.hbase.testclassification.MiscTests; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.apache.hadoop.util.Shell; import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.junit.experimental.categories.Category; @@ -49,6 +50,19 @@ public class TestNodeHealthCheckChore { private File healthScriptFile; private String eol = System.getProperty("line.separator"); + @Before + public void setup() throws IOException { + File tempDir = new File(UTIL.getDataTestDir().toString()); + if (!tempDir.exists()) { + if (!tempDir.mkdirs()) { + throw new IOException("Failed mkdirs " + tempDir); + } + } + String scriptName = "HealthScript" + UUID.randomUUID().toString() + + (Shell.WINDOWS ? ".cmd" : ".sh"); + healthScriptFile = new File(tempDir.getAbsolutePath(), scriptName); + } + @After public void cleanUp() throws IOException { // delete and recreate the test directory, ensuring a clean test dir between tests @@ -78,10 +92,10 @@ public class TestNodeHealthCheckChore { public void healthCheckerTest(String script, HealthCheckerExitStatus expectedStatus) throws Exception { - Configuration config = getConfForNodeHealthScript(); - config.addResource(healthScriptFile.getName()); + ConfigurationManager confManager = getConfManagerForNodeHealthScript(); + confManager.getConfiguration().addResource(healthScriptFile.getName()); String location = healthScriptFile.getAbsolutePath(); - long timeout = config.getLong(HConstants.HEALTH_SCRIPT_TIMEOUT, SCRIPT_TIMEOUT); + long timeout = confManager.getLong(ServerConfigs.HBASE_NODE_HEALTH_SCRIPT_TIMEOUT); HealthChecker checker = new HealthChecker(); checker.init(location, timeout); @@ -98,10 +112,10 @@ public class TestNodeHealthCheckChore { @Test(timeout=60000) public void testRSHealthChore() throws Exception{ Stoppable stop = new StoppableImplementation(); - Configuration conf = getConfForNodeHealthScript(); String errorScript = "echo ERROR" + eol + " echo \"Server not healthy\""; createScript(errorScript, true); - HealthCheckChore rsChore = new HealthCheckChore(100, stop, conf); + HealthCheckChore rsChore = new HealthCheckChore( + 100, stop, getConfManagerForNodeHealthScript()); try { //Default threshold is three. rsChore.chore(); @@ -132,21 +146,13 @@ public class TestNodeHealthCheckChore { LOG.info("Created " + this.healthScriptFile + ", executable=" + setExecutable); } - private Configuration getConfForNodeHealthScript() throws IOException { - Configuration conf = UTIL.getConfiguration(); - File tempDir = new File(UTIL.getDataTestDir().toString()); - if (!tempDir.exists()) { - if (!tempDir.mkdirs()) { - throw new IOException("Failed mkdirs " + tempDir); - } - } - String scriptName = "HealthScript" + UUID.randomUUID().toString() - + (Shell.WINDOWS ? ".cmd" : ".sh"); - healthScriptFile = new File(tempDir.getAbsolutePath(), scriptName); - conf.set(HConstants.HEALTH_SCRIPT_LOC, healthScriptFile.getAbsolutePath()); - conf.setLong(HConstants.HEALTH_FAILURE_THRESHOLD, 3); - conf.setLong(HConstants.HEALTH_SCRIPT_TIMEOUT, SCRIPT_TIMEOUT); - return conf; + private ConfigurationManager getConfManagerForNodeHealthScript() throws IOException { + ConfigurationManager confManager = new ConfigurationManager(UTIL.getConfiguration()); + confManager.setString(ServerConfigs.HBASE_NODE_HEALTH_SCRIPT_LOCATION, + healthScriptFile.getAbsolutePath()); + confManager.setInt(ServerConfigs.HBASE_NODE_HEALTH_FAILURE_THRESHOLD, 3); + confManager.setLong(ServerConfigs.HBASE_NODE_HEALTH_SCRIPT_TIMEOUT, SCRIPT_TIMEOUT); + return confManager; } /** diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaWithReplicas.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaWithReplicas.java index 0e5bd9c..b871649 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaWithReplicas.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaWithReplicas.java @@ -30,6 +30,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Abortable; +import org.apache.hadoop.hbase.CommonConfigs; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.HRegionInfo; @@ -40,6 +41,7 @@ import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.TableNotFoundException; import org.apache.hadoop.hbase.Waiter; +import org.apache.hadoop.hbase.conf.ConfigurationManager; import org.apache.hadoop.hbase.regionserver.StorefileRefresherChore; import org.apache.hadoop.hbase.testclassification.MediumTests; import org.apache.hadoop.hbase.util.Bytes; @@ -148,7 +150,7 @@ public class TestMetaWithReplicas { // location of the test table's region ZooKeeperWatcher zkw = util.getZooKeeperWatcher(); Configuration conf = util.getConfiguration(); - conf.setBoolean(HConstants.USE_META_REPLICAS, true); + conf.setBoolean(CommonConfigs.HBASE_META_REPLICAS_USE.getName(), true); String baseZNode = conf.get(HConstants.ZOOKEEPER_ZNODE_PARENT, HConstants.DEFAULT_ZOOKEEPER_ZNODE_PARENT); @@ -216,7 +218,7 @@ public class TestMetaWithReplicas { util.getHBaseClusterInterface().waitForActiveAndReadyMaster(); ((ClusterConnection)c).clearRegionCache(); htable.close(); - conf.setBoolean(HConstants.USE_META_REPLICAS, false); + conf.setBoolean(CommonConfigs.HBASE_META_REPLICAS_USE.getName(), false); htable = c.getTable(TABLE); r = htable.get(get); assertTrue(Arrays.equals(r.getRow(), row)); @@ -354,8 +356,6 @@ public class TestMetaWithReplicas { @Test public void testAccessingUnknownTables() throws Exception { - Configuration conf = new Configuration(TEST_UTIL.getConfiguration()); - conf.setBoolean(HConstants.USE_META_REPLICAS, true); Table table = TEST_UTIL.getConnection().getTable(TableName.valueOf("RandomTable")); Get get = new Get(Bytes.toBytes("foo")); try { diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/conf/TestConfigurationManager.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/conf/TestConfigurationManager.java index ab4ebc5..e80b451 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/conf/TestConfigurationManager.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/conf/TestConfigurationManager.java @@ -18,21 +18,302 @@ package org.apache.hadoop.hbase.conf; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.ConfigurationProperty; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.SmallTests; +import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.junit.rules.ExpectedException; + +import java.lang.reflect.Field; @Category({SmallTests.class, ClientTests.class}) public class TestConfigurationManager { private static final Log LOG = LogFactory.getLog(TestConfigurationManager.class); + Configuration conf; + ConfigurationManager cm; + + @Rule + public ExpectedException exception = ExpectedException.none(); + + @Before + public void setup() { + // Adds all properties from 'TestConfigs' class to 'conf' using reflection. + conf = new Configuration(); + for (Field field : TestConfigs.class.getFields()) { + ConfigurationProperty property = null; + try { + property = (ConfigurationProperty) field.get(null); + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + conf.set(property.getName(), property.toString()); + } + cm = new ConfigurationManager(conf); + } + + private static class TestConfigs { + public static ConfigurationProperty EMPTY_STRING_PROPERTY = + new ConfigurationProperty<>("empty.string.property", ""); + public static ConfigurationProperty STRING_PROPERTY = + new ConfigurationProperty<>("string.property", "foo"); + public static ConfigurationProperty STRING_ARRAY_PROPERTY = + new ConfigurationProperty<>("string.array.property", + new String[]{"foo", "bar", "pikachu"}); + public static ConfigurationProperty BOOLEAN_PROPERTY = + new ConfigurationProperty<>("boolean.property", false); + public static ConfigurationProperty INT_PROPERTY = + new ConfigurationProperty<>("int.property", 10); + public static ConfigurationProperty LONG_PROPERTY = + new ConfigurationProperty<>("long.property", 1L); + public static ConfigurationProperty FLOAT_PROPERTY = + new ConfigurationProperty<>("float.property", 1.0f); + public static ConfigurationProperty DOUBLE_PROPERTY = + new ConfigurationProperty<>("double.property", 2.0); + } + + @Test + /** + * Tests {@link ConfigurationManager#getString(ConfigurationProperty)}. + * Check default value is returned when value is not set explicitly. + */ + public void testGetString_WhenNotSet_ReturnsDefault() { + assertEquals(TestConfigs.STRING_PROPERTY.getDefaultValue(), + cm.getString(TestConfigs.STRING_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getString(ConfigurationProperty)}. + * Check empty string default value is returned when value is not set explicitly. + */ + public void testGetString_WhenNotSet_ReturnsDefaultEmptyString() { + assertTrue(cm.getString(TestConfigs.EMPTY_STRING_PROPERTY).isEmpty()); + } + + @Test + /** + * Tests {@link ConfigurationManager#getString(ConfigurationProperty)} and + * {@link ConfigurationManager#setString(ConfigurationProperty, String)}. + * Check same value is returned when it is set explicitly. + */ + public void testSetStringAndGetString() { + String nonDefaultValue = TestConfigs.STRING_PROPERTY.getDefaultValue() + "bar"; + cm.setString(TestConfigs.STRING_PROPERTY, nonDefaultValue); + + assertEquals(nonDefaultValue, cm.getString(TestConfigs.STRING_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getBoolean(ConfigurationProperty)}. + * Check default value is returned when value is not set explicitly. + */ + public void testGetBoolean_WhenNotSet_ReturnsDefault() { + assertEquals(TestConfigs.BOOLEAN_PROPERTY.getDefaultValue(), + cm.getBoolean(TestConfigs.BOOLEAN_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getBoolean(ConfigurationProperty)} and + * {@link ConfigurationManager#setBoolean(ConfigurationProperty, boolean)}. + * Check same value is returned when it is set explicitly. + */ + public void testSetBooleanAndGetBoolean() { + boolean nonDefaultValue = !TestConfigs.BOOLEAN_PROPERTY.getDefaultValue(); + cm.setBoolean(TestConfigs.BOOLEAN_PROPERTY, nonDefaultValue); + + assertEquals(nonDefaultValue, cm.getBoolean(TestConfigs.BOOLEAN_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getBoolean(ConfigurationProperty)}. + * When set to invalid value, should return {@link IllegalArgumentException}. + */ + public void testGetBoolean_WhenInvalidValue_ReturnsException() { + conf.set(TestConfigs.BOOLEAN_PROPERTY.getName(), "tru"); + + exception.expect(IllegalArgumentException.class); + cm.getBoolean(TestConfigs.BOOLEAN_PROPERTY); + } + + @Test + /** + * Tests {@link ConfigurationManager#getInt(ConfigurationProperty)}. + * Check default value is returned when value is not set explicitly. + */ + public void testGetInt_WhenNotSet_ReturnsDefault() { + int defaultValue = TestConfigs.INT_PROPERTY.getDefaultValue(); + assertEquals(defaultValue, cm.getInt(TestConfigs.INT_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getInt(ConfigurationProperty)} and + * {@link ConfigurationManager#setInt(ConfigurationProperty, int)}. + * Check same value is returned when it is set explicitly. + */ + public void testSetIntAndGetInt() { + int nonDefaultValue = TestConfigs.INT_PROPERTY.getDefaultValue() + 10; + cm.setInt(TestConfigs.INT_PROPERTY, nonDefaultValue); + + assertEquals(nonDefaultValue, cm.getInt(TestConfigs.INT_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getInt(ConfigurationProperty)}. + * When set to invalid value, should return {@link NumberFormatException}. + */ + public void testGetInt_WhenInvalidValue_ReturnsException() { + conf.set(TestConfigs.INT_PROPERTY.getName(), "abc"); + + exception.expect(NumberFormatException.class); + cm.getInt(TestConfigs.INT_PROPERTY); + } + + @Test + /** + * Tests {@link ConfigurationManager#getLong(ConfigurationProperty)}. + * Check default value is returned when value is not set explicitly. + */ + public void testGetLong_WhenNotSet_ReturnsDefault() { + long defaultValue = TestConfigs.LONG_PROPERTY.getDefaultValue(); + assertEquals(defaultValue, cm.getLong(TestConfigs.LONG_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getLong(ConfigurationProperty)} and + * {@link ConfigurationManager#setLong(ConfigurationProperty, long)}. + * Check same value is returned when it is set explicitly. + */ + public void testSetLongAndGetLong() { + long nonDefaultValue = TestConfigs.LONG_PROPERTY.getDefaultValue() + 10; + cm.setLong(TestConfigs.LONG_PROPERTY, nonDefaultValue); + + assertEquals(nonDefaultValue, cm.getLong(TestConfigs.LONG_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getLong(ConfigurationProperty)}. + * When set to invalid value, should return {@link NumberFormatException}. + */ + public void testGetLong_WhenInvalidValue_ReturnsException() { + conf.set(TestConfigs.LONG_PROPERTY.getName(), "abc"); + + exception.expect(NumberFormatException.class); + cm.getLong(TestConfigs.LONG_PROPERTY); + } + + @Test + /** + * Tests {@link ConfigurationManager#getFloat(ConfigurationProperty)}. + * Check default value is returned when value is not set explicitly. + */ + public void testGetFloat_WhenNotSet_ReturnsDefault() { + assertEquals(TestConfigs.FLOAT_PROPERTY.getDefaultValue(), + cm.getFloat(TestConfigs.FLOAT_PROPERTY), 0.001); + } + + @Test + /** + * Tests {@link ConfigurationManager#getFloat(ConfigurationProperty)} and + * {@link ConfigurationManager#setFloat(ConfigurationProperty, float)}. + * Check same value is returned when it is set explicitly. + */ + public void testSetFloatAndGetFloat() { + float nonDefaultValue = TestConfigs.FLOAT_PROPERTY.getDefaultValue() + 8.88f; + cm.setFloat(TestConfigs.FLOAT_PROPERTY, nonDefaultValue); + + assertEquals(nonDefaultValue, cm.getFloat(TestConfigs.FLOAT_PROPERTY), 0.001); + } + + @Test + /** + * Tests {@link ConfigurationManager#getFloat(ConfigurationProperty)}. + * When set to invalid value, should return {@link NumberFormatException}. + */ + public void testGetFloat_WhenInvalidValue_ReturnsException() { + conf.set(TestConfigs.FLOAT_PROPERTY.getName(), "abc"); + + exception.expect(NumberFormatException.class); + cm.getFloat(TestConfigs.FLOAT_PROPERTY); + } + + @Test + /** + * Tests {@link ConfigurationManager#getDouble(ConfigurationProperty)}. + * Check default value is returned when value is not set explicitly. + */ + public void testGetDouble_WhenNotSet_ReturnsDefault() { + assertEquals(TestConfigs.DOUBLE_PROPERTY.getDefaultValue(), + cm.getDouble(TestConfigs.DOUBLE_PROPERTY), 0.001); + } + + @Test + /** + * Tests {@link ConfigurationManager#getDouble(ConfigurationProperty)} and + * {@link ConfigurationManager#setDouble(ConfigurationProperty, double)}. + * Check same value is returned when it is set explicitly. + */ + public void testSetDoubleGetDouble() { + double nonDefaultValue = TestConfigs.DOUBLE_PROPERTY.getDefaultValue() + 8.88; + cm.setDouble(TestConfigs.DOUBLE_PROPERTY, nonDefaultValue); + + assertEquals(nonDefaultValue, cm.getDouble(TestConfigs.DOUBLE_PROPERTY), 0.001); + } + + @Test + /** + * Tests {@link ConfigurationManager#getDouble(ConfigurationProperty)}. + * When set to invalid value, should return {@link NumberFormatException}. + */ + public void testGetDouble_WhenInvalidValue_ReturnsException() { + conf.set(TestConfigs.DOUBLE_PROPERTY.getName(), "abc"); + + exception.expect(NumberFormatException.class); + cm.getDouble(TestConfigs.DOUBLE_PROPERTY); + } + + @Test + /** + * Tests {@link ConfigurationManager#getStrings(ConfigurationProperty)}. + * Check default value is returned when value is not set explicitly. + */ + public void testGetStrings_WhenNotSet_ReturnsDefault() { + String[] expectedValue = new String[]{"foo", "bar", "pikachu"}; + assertArrayEquals(expectedValue, cm.getStrings(TestConfigs.STRING_ARRAY_PROPERTY)); + } + + @Test + /** + * Tests {@link ConfigurationManager#getStrings(ConfigurationProperty)} and {@link + * ConfigurationManager#setStrings(ConfigurationProperty, String)}. + * Check same value is returned when it is set explicitly. + */ + public void testSetStringsAndGetStrings() { + String[] expectedValue = new String[]{"mario", "luigi"}; + cm.setStrings(TestConfigs.STRING_ARRAY_PROPERTY, expectedValue); + + assertArrayEquals(expectedValue, cm.getStrings(TestConfigs.STRING_ARRAY_PROPERTY)); + } + class DummyConfigurationObserver implements ConfigurationObserver { private boolean notifiedOnChange = false; private ConfigurationManager cm; @@ -71,7 +352,7 @@ public class TestConfigurationManager { @Test public void testCheckIfObserversNotified() { Configuration conf = new Configuration(); - ConfigurationManager cm = new ConfigurationManager(); + ConfigurationManager cm = new ConfigurationManager(conf); DummyConfigurationObserver d1 = new DummyConfigurationObserver(cm); // Check if we get notified. @@ -107,7 +388,7 @@ public class TestConfigurationManager { @Test public void testDeregisterOnOutOfScope() { Configuration conf = new Configuration(); - ConfigurationManager cm = new ConfigurationManager(); + ConfigurationManager cm = new ConfigurationManager(conf); boolean outOfScopeObserversDeregistered = false; diff --git a/pom.xml b/pom.xml index bbb7ff4..b9b90c5 100644 --- a/pom.xml +++ b/pom.xml @@ -1134,6 +1134,19 @@ true true + + org.reflections + reflections-maven + 0.9.9-RC1 + + + + reflections + + process-classes + + + @@ -1745,6 +1758,11 @@ org.jmock jmock-junit4 + + org.reflections + reflections + 0.9.9-RC1 +