You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/11/19 21:49:35 UTC
[GitHub] [accumulo] jmark99 opened a new pull request #1797: Check for consistency of table-related shell commands.
jmark99 opened a new pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797
This ticket examined the table-related shell commands to examine if they provided a means to indicate the table name. The 'insert' command was updated recently to add the ability to supply a target table. This ticket provides the same capability for the 'delete' command. In all, there are 39 shell commands that can take a table name parameter. Each of the 39 now allow a table name to be supplied.
For completeness, the commands are listed here:
deleteiter, deletescaniter, listiter, setiter, setscaniter, grant, revoke, config, deletetable, droptable, du, exporttable, offline, online, summaries, addsplits, compact, constraint, flush, getgroups, getsplits, merge, setgroups, delete, deletemany, deleterows, egrep, formatter, interpreter, grep, importdirectory, insert, maxrow, scan, table, clonetable, createtable, importtable, and renametable.
All but the last six listed above allow the table to be supplied using the -t/--table prefix. The final six do not use of the -t/--table prefix. There are eight that allow the table name to be supplied using both the -t/--table prefix or just the table name (deletetable, droptable, du, offline, online, summaries, compact, and flush).
Would it be worth considering updating the final six to take the -t/--table prefix in order to provide consistency? They could mimic the eight that work with and without the prefix' in order to not break existing workflows. If there is support for doing so, I will create a follow-on ticket to continue the work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on pull request #1797: Add table option to shell delete command.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-731244194
Thanks for catching the missing test cases. Guess I had been using stand alone test scripts that I forgot to transfer them over.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-731189333
For this PR, the best description would be: "Add table option to shell delete command"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-730664661
> Would it be worth considering updating the final six to take the -t/--table prefix in order to provide consistency?
No, I don't think so. For those commands, the table is a required parameter, because they are specifically operating on a table. It's a little confusing that a command might have the table as a required argument, and another command has it as an option, but I think it's even more confusing when there's both an argument *and* an option.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #1797: Add table option to shell delete command.
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-731150147
Updated the original ticket to contain most of the info that was initially in this commit message and trimmed down this commit message to only the work that was done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#discussion_r527709940
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellIT.java
##########
@@ -236,6 +236,17 @@ public void insertDeleteScanTest() throws IOException {
exec("delete \\x90 \\xa0 \\xb0", true);
exec("scan", true, "\\x90 \\xA0:\\xB0 [] \\xC0", false);
exec("deletetable test -f", true, "Table: [test] has been deleted");
+ // Add tests to verify use of --table parameter
+ exec("createtable test2", true);
+ exec("notable", true);
Review comment:
Oh, sorry. I guess I didn't realize we had such a command. I'm guessing you use that to change to a context without a table before proceeding with the test with the table arguments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-730667647
It would be good to add your comment above to the original issue (#1797), and to ensure the log message for the commits in this PR only describe what they did in that specific commit. If this PR closes that issue, it should reference it in its log message somewhere with [one of the closing key words](https://docs.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 edited a comment on pull request #1797: Add table option to shell delete command.
Posted by GitBox <gi...@apache.org>.
jmark99 edited a comment on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-731244194
@ctubbsii, thanks for catching the missing test cases. Guess I had been using stand alone test scripts and forgot to transfer them over.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 edited a comment on pull request #1797: Add table option to shell delete command.
Posted by GitBox <gi...@apache.org>.
jmark99 edited a comment on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-731244194
@ctubbsii, thanks for catching the missing test cases. Guess I had been using stand alone test scripts that I forgot to transfer them over.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#discussion_r527233450
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellIT.java
##########
@@ -236,6 +236,17 @@ public void insertDeleteScanTest() throws IOException {
exec("delete \\x90 \\xa0 \\xb0", true);
exec("scan", true, "\\x90 \\xA0:\\xB0 [] \\xC0", false);
exec("deletetable test -f", true, "Table: [test] has been deleted");
+ // Add tests to verify use of --table parameter
+ exec("createtable test2", true);
+ exec("notable", true);
Review comment:
What is 'notable' here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#issuecomment-731188588
> Updated the original ticket to contain most of the info that was initially in this commit message and trimmed down this commit message to only the work that was done.
I see you edited the description in the ticket, but the commit in the PR still has the longer commit message. You'll want to edit that while merging if you merge from the GitHub website UI.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] jmark99 commented on a change in pull request #1797: Check for consistency of table-related shell commands.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #1797:
URL: https://github.com/apache/accumulo/pull/1797#discussion_r527667743
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellIT.java
##########
@@ -236,6 +236,17 @@ public void insertDeleteScanTest() throws IOException {
exec("delete \\x90 \\xa0 \\xb0", true);
exec("scan", true, "\\x90 \\xA0:\\xB0 [] \\xC0", false);
exec("deletetable test -f", true, "Table: [test] has been deleted");
+ // Add tests to verify use of --table parameter
+ exec("createtable test2", true);
+ exec("notable", true);
Review comment:
It verifies that one can call the 'notable' command without error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org