This is an automated email from the ASF dual-hosted git repository. gjacoby pushed a commit to branch 4.14-HBase-1.4 in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/4.14-HBase-1.4 by this push: new 0654507 PHOENIX-5621 - IndexUpgradeTool uses wrong priority and unnecessary properties for GlobalIndexChecker 0654507 is described below commit 0654507954012e45944d75921fcae5470eafb45d Author: Geoffrey Jacoby <gjac...@apache.org> AuthorDate: Fri Dec 13 11:18:32 2019 -0800 PHOENIX-5621 - IndexUpgradeTool uses wrong priority and unnecessary properties for GlobalIndexChecker --- .../end2end/ParameterizedIndexUpgradeToolIT.java | 27 +++++++----- .../phoenix/end2end/index/IndexCoprocIT.java | 48 ++++++++++++++++++++-- .../phoenix/mapreduce/index/IndexUpgradeTool.java | 17 ++++++-- 3 files changed, 75 insertions(+), 17 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java index 3bf69a1..652ca55 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/ParameterizedIndexUpgradeToolIT.java @@ -18,9 +18,11 @@ package org.apache.phoenix.end2end; import com.google.common.collect.Maps; +import org.apache.hadoop.hbase.HTableDescriptor; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.Admin; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.phoenix.end2end.index.IndexCoprocIT; import org.apache.phoenix.hbase.index.IndexRegionObserver; import org.apache.phoenix.hbase.index.Indexer; import org.apache.phoenix.index.GlobalIndexChecker; @@ -230,18 +232,22 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest { throws IOException { if (mutable) { for (String table : tableList) { + HTableDescriptor indexDesc = admin.getTableDescriptor(TableName.valueOf(table)); Assert.assertTrue("Can't find IndexRegionObserver for " + table, - admin.getTableDescriptor(TableName.valueOf(table)) - .hasCoprocessor(IndexRegionObserver.class.getName())); + indexDesc.hasCoprocessor(IndexRegionObserver.class.getName())); Assert.assertFalse("Found Indexer on " + table, - admin.getTableDescriptor(TableName.valueOf(table)) - .hasCoprocessor(Indexer.class.getName())); + indexDesc.hasCoprocessor(Indexer.class.getName())); + IndexCoprocIT.assertCoprocConfig(indexDesc, IndexRegionObserver.class.getName(), + IndexCoprocIT.INDEX_REGION_OBSERVER_CONFIG); } + } for (String index : indexList) { + HTableDescriptor indexDesc = admin.getTableDescriptor(TableName.valueOf(index)); Assert.assertTrue("Couldn't find GlobalIndexChecker on " + index, - admin.getTableDescriptor(TableName.valueOf(index)) - .hasCoprocessor(GlobalIndexChecker.class.getName())); + indexDesc.hasCoprocessor(GlobalIndexChecker.class.getName())); + IndexCoprocIT.assertCoprocConfig(indexDesc, GlobalIndexChecker.class.getName(), + IndexCoprocIT.GLOBAL_INDEX_CHECKER_CONFIG); } // Transactional indexes should not have new coprocessors for (String index : TRANSACTIONAL_INDEXES_LIST) { @@ -260,12 +266,13 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest { throws IOException { if (mutable) { for (String table : tableList) { + HTableDescriptor indexDesc = admin.getTableDescriptor(TableName.valueOf(table)); Assert.assertTrue("Can't find Indexer for " + table, - admin.getTableDescriptor(TableName.valueOf(table)) - .hasCoprocessor(Indexer.class.getName())); + indexDesc.hasCoprocessor(Indexer.class.getName())); Assert.assertFalse("Found IndexRegionObserver on " + table, - admin.getTableDescriptor(TableName.valueOf(table)) - .hasCoprocessor(IndexRegionObserver.class.getName())); + indexDesc.hasCoprocessor(IndexRegionObserver.class.getName())); + IndexCoprocIT.assertCoprocConfig(indexDesc, Indexer.class.getName(), + IndexCoprocIT.INDEXER_CONFIG); } } for (String index : indexList) { diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexCoprocIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexCoprocIT.java index 0870e37..8dde602 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexCoprocIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/index/IndexCoprocIT.java @@ -22,16 +22,18 @@ import org.apache.hadoop.hbase.HColumnDescriptor; import org.apache.hadoop.hbase.HTableDescriptor; import org.apache.hadoop.hbase.TableName; import org.apache.hadoop.hbase.client.Admin; -import org.apache.hadoop.hbase.client.HTable; +import org.apache.hadoop.hbase.io.ImmutableBytesWritable; import org.apache.hadoop.hbase.util.Bytes; import org.apache.phoenix.end2end.ParallelStatsDisabledIT; import org.apache.phoenix.hbase.index.IndexRegionObserver; import org.apache.phoenix.hbase.index.Indexer; import org.apache.phoenix.hbase.index.covered.NonTxIndexBuilder; import org.apache.phoenix.index.GlobalIndexChecker; +import org.apache.phoenix.index.PhoenixIndexBuilder; import org.apache.phoenix.index.PhoenixIndexCodec; import org.apache.phoenix.jdbc.PhoenixConnection; import org.apache.phoenix.query.QueryServices; +import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.util.SchemaUtil; import org.junit.Assert; import org.junit.Test; @@ -51,6 +53,18 @@ import java.util.Properties; public class IndexCoprocIT extends ParallelStatsDisabledIT { private boolean isNamespaceMapped = false; private boolean isMultiTenant = false; + public static final String GLOBAL_INDEX_CHECKER_CONFIG = + "|org.apache.phoenix.index.GlobalIndexChecker|805306365|"; + public static final String INDEX_REGION_OBSERVER_CONFIG = + "|org.apache.phoenix.hbase.index.IndexRegionObserver" + + "|805306366|org.apache.hadoop.hbase.index.codec.class=" + + "org.apache.phoenix.index.PhoenixIndexCodec," + + "index.builder=org.apache.phoenix.index.PhoenixIndexBuilder"; + public static final String INDEXER_CONFIG = + "|org.apache.phoenix.hbase.index.Indexer" + + "|805306366|org.apache.hadoop.hbase.index.codec.class=" + + "org.apache.phoenix.index.PhoenixIndexCodec," + + "index.builder=org.apache.phoenix.index.PhoenixIndexBuilder"; public IndexCoprocIT(boolean isMultiTenant){ this.isMultiTenant = isMultiTenant; @@ -101,8 +115,8 @@ public class IndexCoprocIT extends ParallelStatsDisabledIT { Map<String, String> props = new HashMap<String, String>(); props.put(NonTxIndexBuilder.CODEC_CLASS_NAME_KEY, PhoenixIndexCodec.class.getName()); - Indexer.enableIndexing(baseDescriptor, NonTxIndexBuilder.class, - props, 100); + Indexer.enableIndexing(baseDescriptor, PhoenixIndexBuilder.class, + props, QueryServicesOptions.DEFAULT_COPROCESSOR_PRIORITY); admin.modifyTable(baseDescriptor.getTableName(), baseDescriptor); } @@ -158,6 +172,8 @@ public class IndexCoprocIT extends ParallelStatsDisabledIT { private void assertUsingOldCoprocs(HTableDescriptor baseDescriptor, HTableDescriptor indexDescriptor) { assertCoprocsContains(Indexer.class, baseDescriptor); + assertCoprocConfig(baseDescriptor, Indexer.class.getName(), + INDEXER_CONFIG); assertCoprocsNotContains(IndexRegionObserver.class, baseDescriptor); assertCoprocsNotContains(IndexRegionObserver.class, indexDescriptor); assertCoprocsNotContains(GlobalIndexChecker.class, indexDescriptor); @@ -171,9 +187,13 @@ public class IndexCoprocIT extends ParallelStatsDisabledIT { private void assertUsingNewCoprocs(HTableDescriptor baseDescriptor, HTableDescriptor indexDescriptor) { assertCoprocsContains(IndexRegionObserver.class, baseDescriptor); + assertCoprocConfig(baseDescriptor, IndexRegionObserver.class.getName(), + INDEX_REGION_OBSERVER_CONFIG); assertCoprocsNotContains(Indexer.class, baseDescriptor); assertCoprocsNotContains(Indexer.class, indexDescriptor); assertCoprocsContains(GlobalIndexChecker.class, indexDescriptor); + assertCoprocConfig(indexDescriptor, GlobalIndexChecker.class.getName(), + GLOBAL_INDEX_CHECKER_CONFIG); } private void assertCoprocsContains(Class clazz, HTableDescriptor descriptor) { @@ -190,6 +210,28 @@ public class IndexCoprocIT extends ParallelStatsDisabledIT { " in descriptor " + descriptor,foundCoproc); } + public static void assertCoprocConfig(HTableDescriptor indexDesc, + String className, String expectedConfigValue){ + boolean foundConfig = false; + for (Map.Entry<ImmutableBytesWritable, ImmutableBytesWritable> entry : + indexDesc.getValues().entrySet()){ + String propKey = Bytes.toString(entry.getKey().get()); + String propValue = Bytes.toString(entry.getValue().get()); + //Unfortunately, a good API to read coproc properties didn't show up until + //HBase 2.0. Doing this the painful String-matching way to be compatible with 1.x + if (propKey.contains("coprocessor")){ + if (propValue.contains(className)){ + Assert.assertEquals(className + " is configured incorrectly", + expectedConfigValue, + propValue); + foundConfig = true; + break; + } + } + } + Assert.assertTrue("Couldn't find config for " + className, foundConfig); + } + private void removeCoproc(Class clazz, HTableDescriptor descriptor, Admin admin) throws Exception { descriptor.removeCoprocessor(clazz.getName()); admin.modifyTable(descriptor.getTableName(), descriptor); diff --git a/phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexUpgradeTool.java b/phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexUpgradeTool.java index bdeaeec..f5e39f4 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexUpgradeTool.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexUpgradeTool.java @@ -19,7 +19,6 @@ package org.apache.phoenix.mapreduce.index; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; -import com.google.gson.Gson; import org.apache.commons.cli.CommandLine; import org.apache.commons.cli.CommandLineParser; import org.apache.commons.cli.HelpFormatter; @@ -109,6 +108,7 @@ public class IndexUpgradeTool extends Configured implements Tool { private HashMap<String, HashSet<String>> tablesAndIndexes = new HashMap<>(); private HashMap<String, HashMap<String,IndexInfo>> rebuildMap = new HashMap<>(); private HashMap<String, String> prop = new HashMap<>(); + private HashMap<String, String> emptyProp = new HashMap<>(); private boolean dryRun, upgrade, syncRebuild; private String operation; @@ -461,11 +461,17 @@ public class IndexUpgradeTool extends Configured implements Tool { } private void addCoprocessor(Admin admin, String tableName, HTableDescriptor tableDesc, - String coprocName) throws IOException { + String coprocName) throws IOException { + addCoprocessor(admin, tableName, tableDesc, coprocName, + QueryServicesOptions.DEFAULT_COPROCESSOR_PRIORITY, prop); + } + + private void addCoprocessor(Admin admin, String tableName, HTableDescriptor tableDesc, + String coprocName, int priority, Map<String, String> propsToAdd) throws IOException { if (!admin.getTableDescriptor(TableName.valueOf(tableName)).hasCoprocessor(coprocName)) { if (!dryRun) { tableDesc.addCoprocessor(coprocName, - null, QueryServicesOptions.DEFAULT_COPROCESSOR_PRIORITY, prop); + null, priority, propsToAdd); } LOGGER.info("Loaded " + coprocName+ " coprocessor on table " + tableName); } else { @@ -491,7 +497,10 @@ public class IndexUpgradeTool extends Configured implements Tool { for (String indexName : indexes) { HTableDescriptor indexTableDesc = admin.getTableDescriptor(TableName.valueOf(indexName)); if (upgrade) { - addCoprocessor(admin, indexName, indexTableDesc, GlobalIndexChecker.class.getName()); + //GlobalIndexChecker needs to be a "lower" priority than all the others so that it + //goes first. It also doesn't get the codec props the IndexRegionObserver needs + addCoprocessor(admin, indexName, indexTableDesc, GlobalIndexChecker.class.getName(), + QueryServicesOptions.DEFAULT_COPROCESSOR_PRIORITY -1, emptyProp); } else { removeCoprocessor(admin, indexName, indexTableDesc, GlobalIndexChecker.class.getName()); }