From 59fffe3f67ebaa98e3dfaa8d519fd80664b232c5 Mon Sep 17 00:00:00 2001 From: Andrew Purtell Date: Sun, 7 Dec 2014 21:13:07 -0800 Subject: [PATCH] HBASE-12575 Sanity check table coprocessor classes are loadable --- .../org/apache/hadoop/hbase/master/HMaster.java | 11 +- .../apache/hadoop/hbase/regionserver/HRegion.java | 10 ++ .../hbase/regionserver/RegionCoprocessorHost.java | 124 +++++++++++++++++---- hbase-shell/src/test/ruby/hbase/admin_test.rb | 22 ++-- 4 files changed, 129 insertions(+), 38 deletions(-) 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 3e96c54..af4abb0 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 @@ -106,6 +106,7 @@ import org.apache.hadoop.hbase.protobuf.generated.ZooKeeperProtos.SplitLogTask.R import org.apache.hadoop.hbase.quotas.MasterQuotaManager; import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.regionserver.RSRpcServices; +import org.apache.hadoop.hbase.regionserver.RegionCoprocessorHost; import org.apache.hadoop.hbase.regionserver.RegionSplitPolicy; import org.apache.hadoop.hbase.replication.regionserver.Replication; import org.apache.hadoop.hbase.security.UserProvider; @@ -1224,9 +1225,9 @@ public class HMaster extends HRegionServer implements MasterServices, Server { + "if you want to bypass sanity checks"); } - // check split policy class can be loaded + // check that coprocessors and other specified plugin classes can be loaded try { - RegionSplitPolicy.getSplitPolicyClass(htd, conf); + checkClassLoading(conf, htd); } catch (Exception ex) { throw new DoNotRetryIOException(ex); } @@ -1379,6 +1380,12 @@ public class HMaster extends HRegionServer implements MasterServices, Server { EncryptionTest.testEncryption(conf, hcd.getEncryptionType(), hcd.getEncryptionKey()); } + private void checkClassLoading(final Configuration conf, final HTableDescriptor htd) + throws IOException { + RegionSplitPolicy.getSplitPolicyClass(htd, conf); + RegionCoprocessorHost.testTableCoprocessorAttrs(conf, htd); + } + private HRegionInfo[] getHRegionInfos(HTableDescriptor hTableDescriptor, byte[][] splitKeys) { long regionId = System.currentTimeMillis(); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index fd5225c..97360d2 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -4853,8 +4853,13 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver { // */ protected HRegion openHRegion(final CancelableProgressable reporter) throws IOException { + // Refuse to open the region if we are missing local compression support checkCompressionCodecs(); + // Refuse to open the region if encryption configuration is incorrect or + // codec support is missing checkEncryption(); + // Refuse to open the region if a required class cannot be loaded + checkClassLoading(); this.openSeqNum = initialize(reporter); this.setSequenceId(openSeqNum); if (wal != null && getRegionServerServices() != null) { @@ -4876,6 +4881,11 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver { // } } + private void checkClassLoading() throws IOException { + RegionSplitPolicy.getSplitPolicyClass(this.htableDescriptor, conf); + RegionCoprocessorHost.testTableCoprocessorAttrs(conf, this.htableDescriptor); + } + /** * Create a daughter region from given a temp directory with the region data. * @param hri Spec. for daughter region to open. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java index e671e50..c179a7f 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java @@ -26,6 +26,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.NavigableSet; +import java.util.UUID; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.BlockingQueue; import java.util.concurrent.ConcurrentHashMap; @@ -36,6 +37,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.protobuf.Message; import com.google.protobuf.Service; + import org.apache.commons.collections.map.AbstractReferenceMap; import org.apache.commons.collections.map.ReferenceMap; import org.apache.commons.logging.Log; @@ -53,6 +55,7 @@ import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HBaseInterfaceAudience; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.HRegionInfo; +import org.apache.hadoop.hbase.HTableDescriptor; import org.apache.hadoop.hbase.client.Append; import org.apache.hadoop.hbase.client.Delete; import org.apache.hadoop.hbase.client.Durability; @@ -80,6 +83,7 @@ import org.apache.hadoop.hbase.regionserver.wal.HLogKey; import org.apache.hadoop.hbase.wal.WALKey; import org.apache.hadoop.hbase.regionserver.wal.WALEdit; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.CoprocessorClassLoader; import org.apache.hadoop.hbase.util.Pair; /** @@ -166,6 +170,36 @@ public class RegionCoprocessorHost } + static class TableCoprocessorAttribute { + private Path path; + private String className; + private int priority; + private Configuration conf; + + public TableCoprocessorAttribute(Path path, String className, int priority, Configuration conf) { + this.path = path; + this.className = className; + this.priority = priority; + this.conf = conf; + } + + public Path getPath() { + return path; + } + + public String getClassName() { + return className; + } + + public int getPriority() { + return priority; + } + + public Configuration getConf() { + return conf; + } + } + /** The region server services */ RegionServerServices rsServices; /** The region */ @@ -197,15 +231,13 @@ public class RegionCoprocessorHost loadTableCoprocessors(conf); } - void loadTableCoprocessors(final Configuration conf) { - // scan the table attributes for coprocessor load specifications - // initialize the coprocessors - List configured = new ArrayList(); - for (Map.Entry e : - region.getTableDesc().getValues().entrySet()) { + static List getTableCoprocessorAttrsFromSchema(Configuration conf, + HTableDescriptor htd) { + List result = Lists.newArrayList(); + for (Map.Entry e: htd.getValues().entrySet()) { String key = Bytes.toString(e.getKey().get()).trim(); - String spec = Bytes.toString(e.getValue().get()).trim(); if (HConstants.CP_HTD_ATTR_KEY_PATTERN.matcher(key).matches()) { + String spec = Bytes.toString(e.getValue().get()).trim(); // found one try { Matcher matcher = HConstants.CP_HTD_ATTR_VALUE_PATTERN.matcher(spec); @@ -215,6 +247,10 @@ public class RegionCoprocessorHost Path path = matcher.group(1).trim().isEmpty() ? null : new Path(matcher.group(1).trim()); String className = matcher.group(2).trim(); + if (className.isEmpty()) { + LOG.error("Malformed table coprocessor specification: key=" + key + ", spec: " + spec); + continue; + } int priority = matcher.group(3).trim().isEmpty() ? Coprocessor.PRIORITY_USER : Integer.valueOf(matcher.group(3)); String cfgSpec = null; @@ -236,20 +272,7 @@ public class RegionCoprocessorHost } else { ourConf = conf; } - // Load encompasses classloading and coprocessor initialization - try { - RegionEnvironment env = load(path, className, priority, ourConf); - configured.add(env); - LOG.info("Loaded coprocessor " + className + " from HTD of " + - region.getTableDesc().getTableName().getNameAsString() + " successfully."); - } catch (Throwable t) { - // Coprocessor failed to load, do we abort on error? - if (conf.getBoolean(ABORT_ON_ERROR_KEY, DEFAULT_ABORT_ON_ERROR)) { - abortServer(className, t); - } else { - LOG.error("Failed to load coprocessor " + className, t); - } - } + result.add(new TableCoprocessorAttribute(path, className, priority, ourConf)); } else { LOG.error("Malformed table coprocessor specification: key=" + key + ", spec: " + spec); @@ -260,6 +283,65 @@ public class RegionCoprocessorHost } } } + return result; + } + + /** + * Sanity check the table coprocessor attributes of the supplied schema. Will + * throw an exception if there is a problem. + * @param conf + * @param htd + * @throws IOException + */ + public static void testTableCoprocessorAttrs(final Configuration conf, + final HTableDescriptor htd) throws IOException { + String pathPrefix = UUID.randomUUID().toString(); + for (TableCoprocessorAttribute attr: getTableCoprocessorAttrsFromSchema(conf, htd)) { + if (attr.getPriority() < 0) { + throw new IOException("Priority for coprocessor " + attr.getClassName() + + " cannot be less than 0"); + } + ClassLoader old = Thread.currentThread().getContextClassLoader(); + try { + ClassLoader cl; + if (attr.getPath() != null) { + cl = CoprocessorClassLoader.getClassLoader(attr.getPath(), + CoprocessorHost.class.getClassLoader(), pathPrefix, conf); + } else { + cl = CoprocessorHost.class.getClassLoader(); + } + Thread.currentThread().setContextClassLoader(cl); + cl.loadClass(attr.getClassName()); + } catch (ClassNotFoundException e) { + throw new IOException("Class " + attr.getClassName() + " cannot be loaded", e); + } finally { + Thread.currentThread().setContextClassLoader(old); + } + } + } + + void loadTableCoprocessors(final Configuration conf) { + // scan the table attributes for coprocessor load specifications + // initialize the coprocessors + List configured = new ArrayList(); + for (TableCoprocessorAttribute attr: getTableCoprocessorAttrsFromSchema(conf, + region.getTableDesc())) { + // Load encompasses classloading and coprocessor initialization + try { + RegionEnvironment env = load(attr.getPath(), attr.getClassName(), attr.getPriority(), + attr.getConf()); + configured.add(env); + LOG.info("Loaded coprocessor " + attr.getClassName() + " from HTD of " + + region.getTableDesc().getTableName().getNameAsString() + " successfully."); + } catch (Throwable t) { + // Coprocessor failed to load, do we abort on error? + if (conf.getBoolean(ABORT_ON_ERROR_KEY, DEFAULT_ABORT_ON_ERROR)) { + abortServer(attr.getClassName(), t); + } else { + LOG.error("Failed to load coprocessor " + attr.getClassName(), t); + } + } + } // add together to coprocessor set for COW efficiency coprocessors.addAll(configured); } diff --git a/hbase-shell/src/test/ruby/hbase/admin_test.rb b/hbase-shell/src/test/ruby/hbase/admin_test.rb index ff2f422..0b12df9 100644 --- a/hbase-shell/src/test/ruby/hbase/admin_test.rb +++ b/hbase-shell/src/test/ruby/hbase/admin_test.rb @@ -310,9 +310,9 @@ module Hbase create_test_table(@test_name) cp_key = "coprocessor" - class_name = "SimpleRegionObserver" + class_name = "org.apache.hadoop.hbase.coprocessor.SimpleRegionObserver" - cp_value = "hdfs:///foo.jar|" + class_name + "|12|arg1=1,arg2=2" + cp_value = "|" + class_name + "|12|arg1=1,arg2=2" # eval() is used to convert a string to regex assert_no_match(eval("/" + class_name + "/"), admin.describe(@test_name)) @@ -326,22 +326,14 @@ module Hbase drop_test_table(@test_name) create_test_table(@test_name) - key1 = "coprocessor" - key2 = "MAX_FILESIZE" - admin.alter(@test_name, true, 'METHOD' => 'table_att', key1 => "|TestCP||") - admin.alter(@test_name, true, 'METHOD' => 'table_att', key2 => 12345678) + key = "MAX_FILESIZE" + admin.alter(@test_name, true, 'METHOD' => 'table_att', key => 12345678) # eval() is used to convert a string to regex - assert_match(eval("/" + key1 + "\\$(\\d+)/"), admin.describe(@test_name)) - assert_match(eval("/" + key2 + "/"), admin.describe(@test_name)) + assert_match(eval("/" + key + "/"), admin.describe(@test_name)) - # get the cp key - cp_keys = admin.describe(@test_name).scan(/(coprocessor\$\d+)/i) - - admin.alter(@test_name, true, 'METHOD' => 'table_att_unset', 'NAME' => cp_keys[0][0]) - admin.alter(@test_name, true, 'METHOD' => 'table_att_unset', 'NAME' => key2) - assert_no_match(eval("/" + key1 + "\\$(\\d+)/"), admin.describe(@test_name)) - assert_no_match(eval("/" + key2 + "/"), admin.describe(@test_name)) + admin.alter(@test_name, true, 'METHOD' => 'table_att_unset', 'NAME' => key) + assert_no_match(eval("/" + key + "/"), admin.describe(@test_name)) end define_test "get_table should get a real table" do -- 1.7.12.4 (Apple Git-37)