ctubbsii commented on code in PR #3684:
URL: https://github.com/apache/accumulo/pull/3684#discussion_r1292928723
##########
shell/src/main/java/org/apache/accumulo/shell/commands/TableOperation.java:
##########
@@ -73,24 +73,12 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
Shell.log.warn("No tables found that match your criteria");
}
- boolean more = true;
- // flush the tables
+ // do op if forced or user answers prompt with yes
for (String tableName : tableSet) {
- if (!more) {
- break;
- }
Review Comment:
The removal of this logic is a slight change in behavior. That's not
necessarily a bad thing, but in this case, I think it's worth preserving the
current behavior. The previous code would interpret a EOF (triggered by Ctrl-C,
or perhaps Ctrl-D ? to abort the prompt) as a "stop asking for the rest"
instruction. With the behavior in this PR, the confirm method will return
false, and continue asking about the rest of the tables.
To solve this, instead of having the confirm method return a boolean, it
could return an `Optional<Boolean>`, with not present generally meaning "no
response" or "abort the action that triggered the question", which could be
used to support this case, or other similar cases.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/MergeCommand.java:
##########
@@ -53,13 +53,9 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
size =
ConfigurationTypeHelper.getFixedMemoryAsBytes(cl.getOptionValue(sizeOpt.getOpt()));
}
if (startRow == null && endRow == null && size < 0 && !all) {
- shellState.getWriter().flush();
- String line = shellState.getReader()
- .readLine("Merge the entire table { " + tableName + " } into one
tablet (yes|no)? ");
- if (line == null) {
- return 0;
- }
- if (!line.equalsIgnoreCase("y") && !line.equalsIgnoreCase("yes")) {
+ if (!shellState
Review Comment:
This is actually a bit confusing, because the force flag was only here to
ensure that we would force merging when the merged tablets would trigger a
subsequent split. The value of the force flag is actually passed to the
underlying implementation, rather than used here. There isn't a force option to
bypass this prompt, as this prompt was added later, based on bad user
experiences merging an entire table.
I think maybe this should be left alone for now, and not allow the force
flag to bypass this question, because it's too important to ensure the user
really wants to trigger the chop compactions that come with merging an entire
table. No automation or force flag should allow that to happen... a decision to
merge an entire table really does need to have user interaction. If somebody
really wants to do it without a prompt, they can automate it in JShell or
compile some Java code to use the API directly.
This prompt will be less important once merging an entire table isn't going
to trigger a bunch of chop compactions, when the no-chop merge feature is
complete.
--
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]
