ctubbsii commented on code in PR #6437:
URL: https://github.com/apache/accumulo/pull/6437#discussion_r3438629695
##########
core/src/main/java/org/apache/accumulo/core/util/Merge.java:
##########
@@ -112,11 +116,18 @@ public void start(String[] args) throws MergeException {
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;
Review Comment:
This gives more information for the current behavior and doesn't change the
current behavior. However, I keep thinking that a goal size of 0 is entirely
valid, and makes much more sense than a goal size of 1 when trying to merge
away consecutive empty tablets.
Having a goal size of zero auto-magically readjust to the tablet split
threshold seems nonsensical to me when an empty size is valid.
I'm glad that this message was added, but I think it might be nice to change
the behavior so that you don't have to do `-s 1` to get an effective goal size
of 0.
##########
shell/src/test/java/org/apache/accumulo/shell/commands/MergeCommandTest.java:
##########
@@ -18,17 +18,128 @@
*/
package org.apache.accumulo.shell.commands;
+import static org.easymock.EasyMock.capture;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.newCapture;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.PrintWriter;
+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.data.Key;
+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.Capture;
+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, boolean dryRun)
+ throws AccumuloException, TableNotFoundException,
AccumuloSecurityException {
+ if (dryRun) {
+ shellState.getWriter()
+ .println(String.format(
+ "dry-run would have started a Fate Merge for table %s tablet
range (%s to %s]",
+ tableName,
+ startRow == null ? "-inf"
+ : Key.toPrintableString(startRow.getBytes(), 0,
startRow.getLength(),
+ startRow.getLength()),
+ endRow == null ? "+inf" :
Key.toPrintableString(endRow.getBytes(), 0,
+ endRow.getLength(), endRow.getLength())));
+ return 0;
+ }
+ 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 mockDryRunMergeTest() throws Exception {
+ MergeCommand cmd = new TestMergeCommand();
+
+ AccumuloClient client = EasyMock.createMock(AccumuloClient.class);
Review Comment:
I think readability would be improved by static import of EasyMock
```suggestion
AccumuloClient client = createMock(AccumuloClient.class);
```
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -96,18 +90,56 @@ public Options getOptions() {
forceOpt = new Option("f", "force", false,
"merge small tablets to large tablets, even if it goes over the given
size");
// Using the constructor does not allow for empty option
- Option.Builder builder = Option.builder().longOpt("all").hasArg(false)
+ Option.Builder allBuilder = Option.builder().longOpt("all").hasArg(false)
.desc("allow an entire table to be merged into one tablet without
prompting"
+ " the user for confirmation");
- allOpt = builder.build();
+ allOpt = allBuilder.build();
+ Option.Builder dryRunBuilder =
Option.builder().longOpt("dry-run").hasArg(false)
+ .desc("will print out the ranges it will merge but not perform any
merge operations.");
Review Comment:
imperative voice works best for command descriptions
```suggestion
.desc("print the ranges it will merge, but do not perform any merge
operations");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
