Skip to content

Move checks to the MergeCommand class #6431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ddanielr wants to merge 4 commits into
base: 2.1
Choose a base branch
from
Draft

Move checks to the MergeCommand class #6431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8946f7
Move checks to the MergeCommand class
ddanielr Jun 16, 2026
f8506d4
Fix name format
ddanielr Jun 16, 2026
6e3a5fa
Adds mergeITs for the metadata table
ddanielr Jun 17, 2026
ec41d9c
fix formatting
ddanielr Jun 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions core/src/main/java/org/apache/accumulo/core/util/Merge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<Long> {
Expand Down Expand Up @@ -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);

@ddanielr ddanielr Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would previously just output a Merging tablets in table <table> to <size> bytes message but would not explicitly state that the size has changed from what was instructed.

The new code change provides a message describing the change in the size selector.

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);
Expand Down Expand Up @@ -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<Size> sizes = new ArrayList<>();
long totalSize = 0;
// Merge any until you get larger than the goal size, and then merge one less tablet
Expand Down Expand Up @@ -239,16 +243,17 @@ protected void merge(AccumuloClient client, String table, List<Size> sizes, int
}
}

protected Iterator<Size> getSizeIterator(AccumuloClient client, String tablename, Text start,
protected Iterator<Size> getSizeIterator(AccumuloClient client, String tableName, Text start,
Text end) throws MergeException {
// open up metadata, walk through the tablets.

TableId tableId;
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())

@ddanielr ddanielr Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the dataLevel of the current table instead of .scanMetadataTable allows the Merge Utility to perform merges against the metadata table.

.overRange(new KeyExtent(tableId, end, start).toMetaRange()).fetch(FILES, PREV_ROW)
.build();
} catch (Exception e) {
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/org/apache/accumulo/core/util/MergeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ static class MergeTester extends Merge {
protected void message(String format, Object... args) {}

@Override
protected Iterator<Size> getSizeIterator(AccumuloClient client, String tablename,
protected Iterator<Size> getSizeIterator(AccumuloClient client, String tableName,
final Text start, final Text end) throws MergeException {
final Iterator<Size> impl = tablets.iterator();
return new Iterator<>() {
Expand Down
50 changes: 35 additions & 15 deletions shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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?!?!?!")
Comment on lines +62 to +63

@dlmarion dlmarion Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge will lock the metadata table for write and then unhost the tablets in the range for some period of time while the merge occurs. The metadata tablets could be unhosted under normal balance conditions, but the system will try to host them immediately. I certainly think we should add a test case for this in MergeIT or some other merge related IT. And I'm wondering if we should add a merge of the metadata table during some Fate operations in some Fate IT.

@ddanielr ddanielr Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test cases for merge operations in MergeIT.

For the FateITs it sounds like you want to see what system interruptions might occur if merging the metadata table happened while other fate operation are ongoing.

@dlmarion dlmarion Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the FateITs it sounds like you want to see what system interruptions might occur if merging the metadata table happened while other fate operation are ongoing.

Exactly. It should be fine as metadata tablets can become unhosted during normal balancing, but it would be good to test this case since we are changing the behavior of merge for the metadata table late in the development cycle in a patch release. In fact, we may want to add that something to the message to say that it's experimental if only to get users to test it first.

.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 { "
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

}
88 changes: 88 additions & 0 deletions shell/src/test/java/org/apache/accumulo/shell/commands/MergeCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,103 @@

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(
new MergeCommand().getOptions().getOption("b").getDescription().contains("(exclusive)"),
"-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);
}
}
Loading