This is an automated email from the ASF dual-hosted git repository. skadam pushed a commit to branch 4.x in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/4.x by this push: new ff77018 PHOENIX-5798 IndexUpgrade tool command line improvements ff77018 is described below commit ff77018d6b09497d1f30d3018ff58b4660b7c59f Author: Tanuj Khurana <khurana.ta...@gmail.com> AuthorDate: Tue Mar 24 07:08:10 2020 -0700 PHOENIX-5798 IndexUpgrade tool command line improvements 1. Add an option to IndexUpgrade tool to optionally rebuild indexes. By default, rebuilding is skipped. 2. Add a placeholder option to IndexUpgrade tool to passthrough the option value to the IndexTool Signed-off-by: s.kadam <s.ka...@apache.org> --- .../end2end/ParameterizedIndexUpgradeToolIT.java | 19 +++-- .../phoenix/mapreduce/index/IndexUpgradeTool.java | 64 +++++++++------ .../apache/phoenix/index/IndexUpgradeToolTest.java | 95 +++++++++++++++++++--- 3 files changed, 133 insertions(+), 45 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 099a2c6..89e9fa9 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 @@ -98,6 +98,7 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest { private final boolean mutable; private final boolean upgrade; private final boolean isNamespaceEnabled; + private final boolean rebuild; private StringBuilder optionsBuilder; private String tableDDLOptions; @@ -136,7 +137,7 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest { admin = queryServices.getAdmin(); iut = new IndexUpgradeTool(upgrade ? UPGRADE_OP : ROLLBACK_OP, INPUT_LIST, null, "/tmp/index_upgrade_" + UUID.randomUUID().toString(), - true, indexToolMock); + true, indexToolMock, rebuild); iut.setConf(getUtility().getConfiguration()); iut.setTest(true); if (!mutable) { @@ -299,20 +300,22 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest { } } - @Parameters(name ="IndexUpgradeToolIT_mutable={0},upgrade={1},isNamespaceEnabled={2}") + @Parameters(name ="IndexUpgradeToolIT_mutable={0},upgrade={1},isNamespaceEnabled={2}, rebuild={3}") public static synchronized Collection<Object[]> data() { return Arrays.asList(new Object[][] { - {false, false, true}, - {true, false, false}, - {false, true, false}, - {true, true, true} + {false, false, true, false}, + {true, false, false, true}, + {false, true, false, false}, + {true, true, true, true} }); } - public ParameterizedIndexUpgradeToolIT(boolean mutable, boolean upgrade, boolean isNamespaceEnabled) { + public ParameterizedIndexUpgradeToolIT(boolean mutable, boolean upgrade, + boolean isNamespaceEnabled, boolean rebuild) { this.mutable = mutable; this.upgrade = upgrade; this.isNamespaceEnabled = isNamespaceEnabled; + this.rebuild = rebuild; } @Test @@ -332,7 +335,7 @@ public class ParameterizedIndexUpgradeToolIT extends BaseTest { Assert.assertEquals("Index upgrade tool waited for client cache to expire " + "for mutable tables", false, iut.getIsWaitComplete()); } - if(upgrade) { + if(upgrade && rebuild) { //verifying if index tool was started Mockito.verify(indexToolMock, times(11)) // for every index/view-index except index on transaction table 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 e81fcb3..322a32f 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 @@ -83,6 +83,9 @@ public class IndexUpgradeTool extends Configured implements Tool { private static final Logger LOGGER = Logger.getLogger(IndexUpgradeTool.class.getName()); + private static final String INDEX_REBUILD_OPTION_SHORT_OPT = "rb"; + private static final String INDEX_TOOL_OPTION_SHORT_OPT = "tool"; + private static final Option OPERATION_OPTION = new Option("o", "operation", true, "[Required] Operation to perform (upgrade/rollback)"); @@ -99,16 +102,16 @@ public class IndexUpgradeTool extends Configured implements Tool { private static final Option LOG_FILE_OPTION = new Option("lf", "logfile", true, "[Optional] Log file path where the logs are written"); - private static final Option INDEX_SYNC_REBUILD_OPTION = new Option("sr", - "index-sync-rebuild", + private static final Option INDEX_REBUILD_OPTION = new Option(INDEX_REBUILD_OPTION_SHORT_OPT, + "index-rebuild", false, - "[Optional] Whether or not synchronously rebuild the indexes; " - + "default rebuild asynchronous"); - - private static final Option INDEX_VERIFY_OPTION = new Option("v", - "verify", + "[Optional] Rebuild the indexes. Set -" + INDEX_TOOL_OPTION_SHORT_OPT + + " to pass options to IndexTool."); + private static final Option INDEX_TOOL_OPTION = new Option(INDEX_TOOL_OPTION_SHORT_OPT, + "index-tool", true, - "[Optional] mode to run indexTool with verify options"); + "[Optional] Options to pass to indexTool when rebuilding indexes. " + + "Set -" + INDEX_REBUILD_OPTION_SHORT_OPT + " to rebuild the index."); public static final String UPGRADE_OP = "upgrade"; public static final String ROLLBACK_OP = "rollback"; @@ -119,12 +122,12 @@ public class IndexUpgradeTool extends Configured implements Tool { private HashMap<String, String> prop = new HashMap<>(); private HashMap<String, String> emptyProp = new HashMap<>(); - private boolean dryRun, upgrade, syncRebuild; + private boolean dryRun, upgrade, rebuild; private String operation; private String inputTables; private String logFile; private String inputFile; - private String verify; + private String indexToolOpts; private boolean test = false; private boolean isWaitComplete = false; @@ -151,8 +154,6 @@ public class IndexUpgradeTool extends Configured implements Tool { public boolean getDryRun() { return this.dryRun; } - public String getVerify() { return this.verify; } - public String getInputTables() { return this.inputTables; } @@ -165,14 +166,19 @@ public class IndexUpgradeTool extends Configured implements Tool { return this.operation; } + public boolean getIsRebuild() { return this.rebuild; } + + public String getIndexToolOpts() { return this.indexToolOpts; } + public IndexUpgradeTool(String mode, String tables, String inputFile, - String outputFile, boolean dryRun, IndexTool indexTool) { + String outputFile, boolean dryRun, IndexTool indexTool, boolean rebuild) { this.operation = mode; this.inputTables = tables; this.inputFile = inputFile; this.logFile = outputFile; this.dryRun = dryRun; this.indexingTool = indexTool; + this.rebuild = rebuild; } public IndexUpgradeTool () { } @@ -234,6 +240,11 @@ public class IndexUpgradeTool extends Configured implements Tool { +TABLE_OPTION.getLongOpt() + " and " + TABLE_CSV_FILE_OPTION.getLongOpt() + "; specify only one."); } + if ((cmdLine.hasOption(INDEX_TOOL_OPTION.getOpt())) + && !cmdLine.hasOption(INDEX_REBUILD_OPTION.getOpt())) { + throw new IllegalStateException("Index tool options should be passed in with " + + INDEX_REBUILD_OPTION.getLongOpt()); + } return cmdLine; } @@ -260,10 +271,10 @@ public class IndexUpgradeTool extends Configured implements Tool { LOG_FILE_OPTION.setOptionalArg(true); options.addOption(LOG_FILE_OPTION); options.addOption(HELP_OPTION); - INDEX_SYNC_REBUILD_OPTION.setOptionalArg(true); - options.addOption(INDEX_SYNC_REBUILD_OPTION); - INDEX_VERIFY_OPTION.setOptionalArg(true); - options.addOption(INDEX_VERIFY_OPTION); + INDEX_REBUILD_OPTION.setOptionalArg(true); + options.addOption(INDEX_REBUILD_OPTION); + INDEX_TOOL_OPTION.setOptionalArg(true); + options.addOption(INDEX_TOOL_OPTION); return options; } @@ -274,8 +285,8 @@ public class IndexUpgradeTool extends Configured implements Tool { logFile = cmdLine.getOptionValue(LOG_FILE_OPTION.getOpt()); inputFile = cmdLine.getOptionValue(TABLE_CSV_FILE_OPTION.getOpt()); dryRun = cmdLine.hasOption(DRY_RUN_OPTION.getOpt()); - syncRebuild = cmdLine.hasOption(INDEX_SYNC_REBUILD_OPTION.getOpt()); - verify = cmdLine.getOptionValue(INDEX_VERIFY_OPTION.getOpt()); + rebuild = cmdLine.hasOption(INDEX_REBUILD_OPTION.getOpt()); + indexToolOpts = cmdLine.getOptionValue(INDEX_TOOL_OPTION.getOpt()); } @VisibleForTesting @@ -573,9 +584,10 @@ public class IndexUpgradeTool extends Configured implements Tool { } private void rebuildIndexes(Connection conn, Configuration conf, ArrayList<String> tableList) { - if (!upgrade) { + if (!upgrade || !rebuild) { return; } + for (String table: tableList) { rebuildIndexes(conn, conf, table); } @@ -710,12 +722,12 @@ public class IndexUpgradeTool extends Configured implements Tool { list.add("-tenant"); list.add(tenantId); } - if (syncRebuild) { - list.add("-runfg"); - } - if(!Strings.isNullOrEmpty(verify)) { - list.add("-v"); - list.add(verify); + + if (!Strings.isNullOrEmpty(indexToolOpts)) { + String[] options = indexToolOpts.split("\\s+"); + for (String opt : options) { + list.add(opt); + } } return list.toArray(new String[list.size()]); } diff --git a/phoenix-core/src/test/java/org/apache/phoenix/index/IndexUpgradeToolTest.java b/phoenix-core/src/test/java/org/apache/phoenix/index/IndexUpgradeToolTest.java index 9554aff..a87a4e0 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/index/IndexUpgradeToolTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/index/IndexUpgradeToolTest.java @@ -31,13 +31,13 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HConstants; +import org.apache.phoenix.mapreduce.index.IndexTool; import org.apache.phoenix.mapreduce.index.IndexUpgradeTool; import org.apache.phoenix.query.QueryServices; import org.apache.phoenix.query.QueryServicesOptions; import org.apache.phoenix.util.PhoenixRuntime; import org.junit.Assert; -import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -50,18 +50,16 @@ public class IndexUpgradeToolTest { private final boolean upgrade; private static final String DUMMY_STRING_VALUE = "anyValue"; private static final String DUMMY_VERIFY_VALUE = "someVerifyValue"; + private static final String ONLY_VERIFY_VALUE = "ONLY"; private IndexUpgradeTool indexUpgradeTool=null; private String outputFile; public IndexUpgradeToolTest(boolean upgrade) { this.upgrade = upgrade; + this.outputFile = "/tmp/index_upgrade_" + UUID.randomUUID().toString(); } - @Before - public void setup() { - outputFile = "/tmp/index_upgrade_" + UUID.randomUUID().toString(); - String [] args = {"-o", upgrade ? UPGRADE_OP : ROLLBACK_OP, "-tb", - INPUT_LIST, "-lf", outputFile, "-d", "-v", DUMMY_VERIFY_VALUE}; + private void setup(String[] args) { indexUpgradeTool = new IndexUpgradeTool(); CommandLine cmd = indexUpgradeTool.parseOptions(args); indexUpgradeTool.initializeTool(cmd); @@ -69,26 +67,101 @@ public class IndexUpgradeToolTest { @Test public void testCommandLineParsing() { + String [] args = {"-o", upgrade ? UPGRADE_OP : ROLLBACK_OP, "-tb", + INPUT_LIST, "-lf", outputFile, "-d"}; + setup(args); Assert.assertEquals(indexUpgradeTool.getDryRun(),true); Assert.assertEquals(indexUpgradeTool.getInputTables(), INPUT_LIST); Assert.assertEquals(indexUpgradeTool.getOperation(), upgrade ? UPGRADE_OP : ROLLBACK_OP); Assert.assertEquals(indexUpgradeTool.getLogFile(), outputFile); + // verify index rebuild is disabled by default + Assert.assertEquals(false, indexUpgradeTool.getIsRebuild()); + Assert.assertNull(indexUpgradeTool.getIndexToolOpts()); } @Test - public void testIfVerifyOptionIsPassedToTool() { + public void testRebuildOptionParsing() { + String [] args = {"-o", upgrade ? UPGRADE_OP : ROLLBACK_OP, "-tb", + INPUT_LIST, "-rb"}; + setup(args); + Assert.assertEquals(true, indexUpgradeTool.getIsRebuild()); + Assert.assertNull(indexUpgradeTool.getIndexToolOpts()); + } + + @Test(expected = IllegalStateException.class) + public void testIndexToolOptionsNoRebuild() { + String indexToolOpts = "-v " + DUMMY_VERIFY_VALUE; + String [] args = {"-o", upgrade ? UPGRADE_OP : ROLLBACK_OP, "-tb", INPUT_LIST, + "-tool", indexToolOpts}; + setup(args); + } + + @Test + public void testIfOptionsArePassedToIndexTool() { if (!upgrade) { return; } - Assert.assertEquals("value passed with verify option does not match with provided value", - DUMMY_VERIFY_VALUE, indexUpgradeTool.getVerify()); + String [] indexToolOpts = {"-v", ONLY_VERIFY_VALUE, "-runfg", "-st", "100"}; + String indexToolarg = String.join(" ", indexToolOpts); + String [] args = {"-o", upgrade ? UPGRADE_OP : ROLLBACK_OP, "-tb", + INPUT_LIST, "-lf", outputFile, "-d", "-rb", "-tool", indexToolarg }; + setup(args); + + Assert.assertEquals("value passed to index tool option does not match with provided value", + indexToolarg, indexUpgradeTool.getIndexToolOpts()); String [] values = indexUpgradeTool.getIndexToolArgValues(DUMMY_STRING_VALUE, DUMMY_STRING_VALUE, DUMMY_STRING_VALUE, DUMMY_STRING_VALUE, DUMMY_STRING_VALUE); List<String> argList = Arrays.asList(values); - Assert.assertTrue(argList.contains(DUMMY_VERIFY_VALUE)); Assert.assertTrue(argList.contains("-v")); + Assert.assertTrue(argList.contains(ONLY_VERIFY_VALUE)); Assert.assertEquals("verify option and value are not passed consecutively", 1, - argList.indexOf(DUMMY_VERIFY_VALUE) - argList.indexOf("-v")); + argList.indexOf(ONLY_VERIFY_VALUE) - argList.indexOf("-v")); + Assert.assertTrue(argList.contains("-runfg")); + Assert.assertTrue(argList.contains("-st")); + + // ensure that index tool can parse the options and raises no exceptions + IndexTool it = new IndexTool(); + CommandLine commandLine = it.parseOptions(values); + it.populateIndexToolAttributes(commandLine); + } + + @Test + public void testMalformedSpacingOptionsArePassedToIndexTool() { + if (!upgrade) { + return; + } + String [] indexToolOpts = {"-v"+ONLY_VERIFY_VALUE, " -runfg", " -st ", "100 "}; + String indexToolarg = String.join(" ", indexToolOpts); + String [] args = {"-o", upgrade ? UPGRADE_OP : ROLLBACK_OP, "-tb", + INPUT_LIST, "-rb", "-tool", indexToolarg }; + setup(args); + + Assert.assertEquals("value passed to index tool option does not match with provided value", + indexToolarg, indexUpgradeTool.getIndexToolOpts()); + String [] values = indexUpgradeTool.getIndexToolArgValues(DUMMY_STRING_VALUE, + DUMMY_STRING_VALUE, DUMMY_STRING_VALUE, DUMMY_STRING_VALUE, DUMMY_STRING_VALUE); + List<String> argList = Arrays.asList(values); + Assert.assertTrue(argList.contains("-v" + ONLY_VERIFY_VALUE)); + Assert.assertTrue(argList.contains("-runfg")); + Assert.assertTrue(argList.contains("-st")); + + // ensure that index tool can parse the options and raises no exceptions + IndexTool it = new IndexTool(); + CommandLine commandLine = it.parseOptions(values); + it.populateIndexToolAttributes(commandLine); + } + + @Test(expected = IllegalStateException.class) + public void testBadIndexToolOptions() { + String [] indexToolOpts = {"-v" + DUMMY_VERIFY_VALUE}; + String indexToolarg = String.join(" ", indexToolOpts); + String [] args = {"-o", UPGRADE_OP, "-tb", INPUT_LIST, "-rb", "-tool", indexToolarg }; + setup(args); + String [] values = indexUpgradeTool.getIndexToolArgValues(DUMMY_STRING_VALUE, + DUMMY_STRING_VALUE, DUMMY_STRING_VALUE, DUMMY_STRING_VALUE, DUMMY_STRING_VALUE); + IndexTool it = new IndexTool(); + CommandLine commandLine = it.parseOptions(values); + it.populateIndexToolAttributes(commandLine); } @Parameters(name ="IndexUpgradeToolTest_mutable={1}")