This is an automated email from the ASF dual-hosted git repository. gjacoby pushed a commit to branch 4.x-HBase-1.5 in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/4.x-HBase-1.5 by this push: new a2adf5e PHOENIX-5621 - IndexUpgradeTool uses wrong priority and unnecessary properties for GlobalIndexChecker a2adf5e is described below commit a2adf5e572c5a4bcccee7f8ac43bad6b84293ec6 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 1df46a8..16f99e3 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 cdf45e2..839d46b 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; @@ -86,8 +100,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); baseDescriptor = admin.getTableDescriptor(TableName.valueOf(physicalTableName)); indexDescriptor = admin.getTableDescriptor(TableName.valueOf(physicalIndexName)); @@ -153,6 +167,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); @@ -166,9 +182,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) { @@ -196,6 +216,28 @@ public class IndexCoprocIT extends ParallelStatsDisabledIT { return 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 437738a..ef8c492 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; @@ -108,6 +107,7 @@ public class IndexUpgradeTool extends Configured implements Tool { private HashMap<String, HashSet<String>> tablesAndIndexes = 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 { @@ -490,7 +496,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());