diff --git a/core/src/main/java/org/apache/accumulo/core/util/Merge.java b/core/src/main/java/org/apache/accumulo/core/util/Merge.java index 41f5a67943b..40ff92a2d00 100644 --- a/core/src/main/java/org/apache/accumulo/core/util/Merge.java +++ b/core/src/main/java/org/apache/accumulo/core/util/Merge.java @@ -36,7 +36,8 @@ import org.apache.accumulo.core.data.Key; import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.dataImpl.KeyExtent; -import org.apache.accumulo.core.metadata.MetadataTable; +import org.apache.accumulo.core.metadata.RootTable; +import org.apache.accumulo.core.metadata.schema.Ample.DataLevel; import org.apache.accumulo.core.metadata.schema.DataFileValue; import org.apache.accumulo.core.metadata.schema.TabletsMetadata; import org.apache.accumulo.core.trace.TraceUtil; @@ -63,7 +64,7 @@ public static class MergeException extends Exception { private static final Logger log = LoggerFactory.getLogger(Merge.class); protected void message(String format, Object... args) { - log.info(String.format(format, args)); + log.info("{}", String.format(format, args)); } public static class MemoryConverter implements IStringConverter { @@ -109,10 +110,16 @@ public void start(String[] args) throws MergeException { System.err.println("table " + opts.tableName + " does not exist"); return; } + if (RootTable.NAME.equals(opts.tableName)) { + throw new IllegalArgumentException("Cannot merge the root table"); + } if (opts.goalSize == null || opts.goalSize < 1) { AccumuloConfiguration tableConfig = new ConfigurationCopy(client.tableOperations().getConfiguration(opts.tableName)); - opts.goalSize = tableConfig.getAsBytes(Property.TABLE_SPLIT_THRESHOLD); + long newGoalSize = tableConfig.getAsBytes(Property.TABLE_SPLIT_THRESHOLD); + message("Invalid goal size: " + opts.goalSize + " Using the " + + Property.TABLE_SPLIT_THRESHOLD.getKey() + " value of : " + newGoalSize); + opts.goalSize = newGoalSize; } message("Merging tablets in table %s to %d bytes", opts.tableName, opts.goalSize); @@ -144,9 +151,6 @@ public Size(KeyExtent extent, long size) { public void mergomatic(AccumuloClient client, String table, Text start, Text end, long goalSize, boolean force) throws MergeException { try { - if (table.equals(MetadataTable.NAME)) { - throw new IllegalArgumentException("cannot merge tablets on the metadata table"); - } List sizes = new ArrayList<>(); long totalSize = 0; // Merge any until you get larger than the goal size, and then merge one less tablet @@ -239,7 +243,7 @@ protected void merge(AccumuloClient client, String table, List sizes, int } } - protected Iterator getSizeIterator(AccumuloClient client, String tablename, Text start, + protected Iterator getSizeIterator(AccumuloClient client, String tableName, Text start, Text end) throws MergeException { // open up metadata, walk through the tablets. @@ -247,8 +251,9 @@ protected Iterator getSizeIterator(AccumuloClient client, String tablename TabletsMetadata tablets; try { ClientContext context = (ClientContext) client; - tableId = context.getTableId(tablename); - tablets = TabletsMetadata.builder(context).scanMetadataTable() + tableId = context.getTableId(tableName); + DataLevel dataLevel = DataLevel.of(tableId); + tablets = TabletsMetadata.builder(context).scanTable(dataLevel.metaTable()) .overRange(new KeyExtent(tableId, end, start).toMetaRange()).fetch(FILES, PREV_ROW) .build(); } catch (Exception e) { diff --git a/core/src/test/java/org/apache/accumulo/core/util/MergeTest.java b/core/src/test/java/org/apache/accumulo/core/util/MergeTest.java index 6d253fe0ae2..856ff0ebd78 100644 --- a/core/src/test/java/org/apache/accumulo/core/util/MergeTest.java +++ b/core/src/test/java/org/apache/accumulo/core/util/MergeTest.java @@ -57,7 +57,7 @@ static class MergeTester extends Merge { protected void message(String format, Object... args) {} @Override - protected Iterator getSizeIterator(AccumuloClient client, String tablename, + protected Iterator getSizeIterator(AccumuloClient client, String tableName, final Text start, final Text end) throws MergeException { final Iterator impl = tablets.iterator(); return new Iterator<>() { diff --git a/shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java b/shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java index d6a74fce231..ae8dfddddd2 100644 --- a/shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java +++ b/shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java @@ -18,7 +18,11 @@ */ package org.apache.accumulo.shell.commands; +import org.apache.accumulo.core.client.AccumuloException; +import org.apache.accumulo.core.client.AccumuloSecurityException; +import org.apache.accumulo.core.client.TableNotFoundException; import org.apache.accumulo.core.conf.ConfigurationTypeHelper; +import org.apache.accumulo.core.metadata.MetadataTable; import org.apache.accumulo.core.util.Merge; import org.apache.accumulo.shell.Shell; import org.apache.accumulo.shell.Shell.Command; @@ -52,6 +56,15 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s if (cl.hasOption(sizeOpt.getOpt())) { size = ConfigurationTypeHelper.getFixedMemoryAsBytes(cl.getOptionValue(sizeOpt.getOpt())); } + if (tableName.equals(MetadataTable.NAME)) { + if (!shellState + .confirm( + " Warning!!! Merging the " + MetadataTable.NAME + " table incorrectly can result in " + + "system instability. Are you REALLY sure you want to merge?!?!?!") + .orElse(false)) { + return 0; + } + } if (startRow == null && endRow == null && size < 0 && !all) { if (!shellState .confirm(" Warning!!! Are you REALLY sure you want to merge the entire table { " @@ -60,21 +73,7 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s return 0; } } - if (size < 0) { - shellState.getAccumuloClient().tableOperations().merge(tableName, startRow, endRow); - } else { - final boolean finalVerbose = verbose; - final Merge merge = new Merge() { - @Override - protected void message(String fmt, Object... args) { - if (finalVerbose) { - shellState.getWriter().println(String.format(fmt, args)); - } - } - }; - merge.mergomatic(shellState.getAccumuloClient(), tableName, startRow, endRow, size, force); - } - return 0; + return executeMerge(shellState, tableName, startRow, endRow, size, verbose, force); } @Override @@ -110,4 +109,25 @@ public Options getOptions() { return o; } + // This method is stubbed out to allow for mock testing + int executeMerge(Shell shellState, String tableName, Text startRow, Text endRow, long size, + boolean verbose, boolean force) throws AccumuloException, TableNotFoundException, + AccumuloSecurityException, Merge.MergeException { + if (size < 0) { + shellState.getAccumuloClient().tableOperations().merge(tableName, startRow, endRow); + } else { + final boolean finalVerbose = verbose; + final Merge merge = new Merge() { + @Override + protected void message(String fmt, Object... args) { + if (finalVerbose) { + shellState.getWriter().println(String.format(fmt, args)); + } + } + }; + merge.mergomatic(shellState.getAccumuloClient(), tableName, startRow, endRow, size, force); + } + return 0; + } + } diff --git a/shell/src/test/java/org/apache/accumulo/shell/commands/MergeCommandTest.java b/shell/src/test/java/org/apache/accumulo/shell/commands/MergeCommandTest.java index 42c85f30a33..9b69e7bcd79 100644 --- a/shell/src/test/java/org/apache/accumulo/shell/commands/MergeCommandTest.java +++ b/shell/src/test/java/org/apache/accumulo/shell/commands/MergeCommandTest.java @@ -20,10 +20,39 @@ import static org.junit.jupiter.api.Assertions.assertTrue; +import java.util.Optional; + +import org.apache.accumulo.core.client.AccumuloClient; +import org.apache.accumulo.core.client.AccumuloException; +import org.apache.accumulo.core.client.AccumuloSecurityException; +import org.apache.accumulo.core.client.TableNotFoundException; +import org.apache.accumulo.core.client.admin.InstanceOperations; +import org.apache.accumulo.core.client.admin.TableOperations; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.metadata.MetadataTable; +import org.apache.accumulo.shell.Shell; +import org.apache.commons.cli.CommandLine; +import org.apache.commons.cli.CommandLineParser; +import org.apache.commons.cli.DefaultParser; +import org.apache.commons.cli.Options; +import org.apache.hadoop.io.Text; +import org.easymock.EasyMock; import org.junit.jupiter.api.Test; public class MergeCommandTest { + public static class TestMergeCommand extends MergeCommand { + @Override + int executeMerge(Shell shellState, String tableName, Text startRow, Text endRow, long size, + boolean verbose, boolean force) + throws AccumuloException, TableNotFoundException, AccumuloSecurityException { + if (size > 0) { + shellState.getAccumuloClient().tableOperations().merge(tableName, startRow, endRow); + } + return 0; + } + } + @Test public void testBeginRowHelp() { assertTrue( @@ -31,4 +60,63 @@ public void testBeginRowHelp() { "-b should say it is exclusive"); } + @Test + public void mockMetadataMergeTest() throws Exception { + MergeCommand cmd = new TestMergeCommand(); + + AccumuloClient client = EasyMock.createMock(AccumuloClient.class); + ClientContext context = EasyMock.createMock(ClientContext.class); + TableOperations tableOps = EasyMock.createMock(TableOperations.class); + InstanceOperations instOps = EasyMock.createMock(InstanceOperations.class); + Shell shellState = EasyMock.createMock(Shell.class); + + Options opts = cmd.getOptions(); + + CommandLineParser parser = new DefaultParser(); + String[] args = {"-t", MetadataTable.NAME, "-s", "0"}; + CommandLine cli = parser.parse(opts, args); + + EasyMock.expect(shellState.getAccumuloClient()).andReturn(client).anyTimes(); + EasyMock.expect(shellState.getContext()).andReturn(context).anyTimes(); + EasyMock.expect(shellState.isVerbose()).andReturn(false).anyTimes(); + EasyMock.expect(client.tableOperations()).andReturn(tableOps).anyTimes(); + EasyMock.expect(tableOps.exists(MetadataTable.NAME)).andReturn(true).anyTimes(); + EasyMock.expect(shellState.confirm( + " Warning!!! Merging the accumulo.metadata table incorrectly can result in system instability. Are you REALLY sure you want to merge?!?!?!")) + .andReturn(Optional.of(true)).once(); + + EasyMock.replay(client, context, tableOps, instOps, shellState); + cmd.execute("merge", cli, shellState); + EasyMock.verify(client, context, tableOps, instOps, shellState); + } + + @Test + public void mockMergeAllTabletsTest() throws Exception { + MergeCommand cmd = new TestMergeCommand(); + + AccumuloClient client = EasyMock.createMock(AccumuloClient.class); + ClientContext context = EasyMock.createMock(ClientContext.class); + TableOperations tableOps = EasyMock.createMock(TableOperations.class); + InstanceOperations instOps = EasyMock.createMock(InstanceOperations.class); + Shell shellState = EasyMock.createMock(Shell.class); + + Options opts = cmd.getOptions(); + + CommandLineParser parser = new DefaultParser(); + String[] args = {"-t", "testTable"}; + CommandLine cli = parser.parse(opts, args); + + EasyMock.expect(shellState.getAccumuloClient()).andReturn(client).anyTimes(); + EasyMock.expect(shellState.getContext()).andReturn(context).anyTimes(); + EasyMock.expect(shellState.isVerbose()).andReturn(false).anyTimes(); + EasyMock.expect(client.tableOperations()).andReturn(tableOps).anyTimes(); + EasyMock.expect(tableOps.exists("testTable")).andReturn(true).anyTimes(); + EasyMock.expect(shellState.confirm( + " Warning!!! Are you REALLY sure you want to merge the entire table { testTable } into one tablet?!?!?!")) + .andReturn(Optional.of(true)).once(); + + EasyMock.replay(client, context, tableOps, instOps, shellState); + cmd.execute("merge", cli, shellState); + EasyMock.verify(client, context, tableOps, instOps, shellState); + } } diff --git a/test/src/main/java/org/apache/accumulo/test/functional/MergeIT.java b/test/src/main/java/org/apache/accumulo/test/functional/MergeIT.java index 43590e80307..b908f0bb789 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/MergeIT.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/MergeIT.java @@ -21,9 +21,12 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.time.Duration; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.HashSet; @@ -45,6 +48,8 @@ import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.data.Value; import org.apache.accumulo.core.dataImpl.KeyExtent; +import org.apache.accumulo.core.metadata.MetadataTable; +import org.apache.accumulo.core.metadata.RootTable; import org.apache.accumulo.core.metadata.StoredTabletFile; import org.apache.accumulo.core.metadata.TabletFile; import org.apache.accumulo.core.metadata.schema.ExternalCompactionId; @@ -278,4 +283,124 @@ public void testCompactionMetadata() throws Exception { } } } + + @Test + public void testMetadataTableMergesSuccessfully() throws Exception { + try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) { + + c.tableOperations().addSplits(MetadataTable.NAME, + new TreeSet<>(List.of(new Text("~del"), new Text("~sserv"), new Text("~err")))); + + String tableName = getUniqueNames(1)[0]; + c.tableOperations().create(tableName); + TableId tableId = getServerContext().getTableId(tableName); + try (BatchWriter writer = c.createBatchWriter(tableName)) { + Mutation m = new Mutation("row"); + m.put("cf", "cq", "NonNull Value"); + writer.addMutation(m); + } + c.tableOperations().flush(tableName, null, null, true); + + // Grab the current number of tablets + long expectedTablets = getServerContext().getAmple().readTablets().forTable(MetadataTable.ID) + .build().stream().count(); + // We have added two adjacent splits, so the number of tablets should now be increased by 2. + c.tableOperations().addSplits(MetadataTable.NAME, new TreeSet<>( + List.of(new Text(tableId.canonical()), new Text(tableId.canonical() + "<")))); + + try (var tablets = + getServerContext().getAmple().readTablets().forTable(MetadataTable.ID).build()) { + assertEquals(expectedTablets + 2, tablets.stream().count()); + } + + List args = new ArrayList<>(List.of("-t", MetadataTable.NAME, "-e", "~")); + getClientProps().stringPropertyNames().forEach(keyProp -> { + args.add("-o"); + args.add(keyProp + "=" + getClientProps().getProperty(keyProp)); + }); + Merge.main(args.toArray(String[]::new)); + try (var tablets = + getServerContext().getAmple().readTablets().forTable(MetadataTable.ID).build()) { + assertEquals(expectedTablets, tablets.stream().count()); + } + } + } + + @Test + public void testMetadataTableSingleByteLimit() throws Exception { + try (AccumuloClient c = Accumulo.newClient().from(getClientProps()).build()) { + String tableName = getUniqueNames(1)[0]; + c.tableOperations().create(tableName); + TableId tableId = getServerContext().getTableId(tableName); + try (BatchWriter writer = c.createBatchWriter(tableName)) { + Mutation m = new Mutation("row"); + m.put("cf", "cq", "NonNull Value"); + writer.addMutation(m); + } + c.tableOperations().flush(tableName, null, null, true); + + // Grab the current number of tablets + long expectedTablets = getServerContext().getAmple().readTablets().forTable(MetadataTable.ID) + .build().stream().count(); + // We have added two adjacent splits, so the number of tablets should now be increased by 2. + c.tableOperations().addSplits(MetadataTable.NAME, new TreeSet<>( + List.of(new Text(tableId.canonical()), new Text(tableId.canonical() + "<")))); + + try (var tablets = + getServerContext().getAmple().readTablets().forTable(MetadataTable.ID).build()) { + assertEquals(expectedTablets + 2, tablets.stream().count()); + } + + List args = new ArrayList<>(List.of("-t", MetadataTable.NAME, "-e", "~", "-s", "1")); + getClientProps().stringPropertyNames().forEach(keyProp -> { + args.add("-o"); + args.add(keyProp + "=" + getClientProps().getProperty(keyProp)); + }); + Merge.main(args.toArray(String[]::new)); + try (var tablets = + getServerContext().getAmple().readTablets().forTable(MetadataTable.ID).build()) { + assertEquals(expectedTablets + 2, tablets.stream().count()); + } + // Add splits for zero length entries in the ~del markers + c.tableOperations().addSplits(MetadataTable.NAME, new TreeSet<>( + List.of(new Text("~del0"), new Text("~del1"), new Text("~del2"), new Text("~del3")))); + try (var tablets = + getServerContext().getAmple().readTablets().forTable(MetadataTable.ID).build()) { + assertEquals(expectedTablets + 6, tablets.stream().count()); + } + // Perform a merge that will collapse the empty tablets into a single "~del" tablet. + List newArgs = + new ArrayList<>(List.of("-t", MetadataTable.NAME, "-b", "~", "-e", "~del9", "-s", "1")); + getClientProps().stringPropertyNames().forEach(keyProp -> { + newArgs.add("-o"); + newArgs.add(keyProp + "=" + getClientProps().getProperty(keyProp)); + }); + Merge.main(newArgs.toArray(String[]::new)); + try (var tablets = + getServerContext().getAmple().readTablets().forTable(MetadataTable.ID).build()) { + for (var tablet : tablets) { + System.out.println("Row Extent" + tablet.getExtent()); + } + // ~del3 is now an empty tablet and cannot be merged with the ~ tablet that is over 1 byte + // in size + assertEquals(expectedTablets + 3, tablets.stream().count()); + } + } + } + + @Test + public void testRootTableDoesNotMerge() { + List args = new ArrayList<>(List.of("-t", RootTable.NAME)); + getClientProps().stringPropertyNames().forEach(keyProp -> { + args.add("-o"); + args.add(keyProp + "=" + getClientProps().getProperty(keyProp)); + }); + Exception e = + assertThrows(Merge.MergeException.class, () -> Merge.main(args.toArray(String[]::new))); + assertInstanceOf(IllegalArgumentException.class, e.getCause()); + assertTrue(e.getMessage().contains("Cannot merge the root table")); + try (var tablets = getServerContext().getAmple().readTablets().forTable(RootTable.ID).build()) { + assertEquals(1, tablets.stream().count()); + } + } }