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/06 18:25:57 UTC
[GitHub] [accumulo] jmark99 opened a new pull request #1771: Add tablename option to shell insert command.
jmark99 opened a new pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771
This change adds the <code>--tablename</code> option to the insert command when inside the shell or when using the accumulo command with <code>accumulo shell -e \<task\></code>. The option is not required, but if used it allows data to be inserted into a table in situations where the shell is not within a table context or when in a table context to insert data into another table.
Although I would not expect this option to be utilized very often, I found myself in need of it when working with the various accumulo examples in the accumulo-examples repo. I had many situations where I wished to insert some test data from a bash shell using the form:
<code>accumulo shell -u user -p pwd -e "insert r f q v"</code>
but this would fail due to there being no table name context for the insertion. This update allows data inserts from a command line using the form:
<code>accumulo shell -u user -p pwd -e "insert r f q v --tablename destination_table" </code>
Note, there is no shortened form of the option to minimize confusion with the -ts 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] EdColeman commented on pull request #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#issuecomment-724923053
I realize this has been merged - but I have a more general question. This is at least the third time that a table option has been added to a shell command (I added it for importdir, there was another, but I don't recall specifics) and then this one. Would it be worth creating an issue and checking that we are consistent across all of the commands that operate on a table that they accept this as 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] jmark99 commented on pull request #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#issuecomment-726055015
Hi Ed,
I think that would be a good idea. Unless you wish to pursue it, I would be willing to take a look since I’m currently reviewing the Accumulo-examples code and coming across various commands that may need the option. Let me know which you would prefer.
Mark
From: EdColeman <no...@github.com>
Sent: Tuesday, November 10, 2020 2:40 PM
To: apache/accumulo <ac...@noreply.github.com>
Cc: Mark Owens <jm...@apache.org>; State change <st...@noreply.github.com>
Subject: Re: [apache/accumulo] Add tablename option to shell insert command. (#1771)
I realize this has been merged - but I have a more general question. This is at least the third time that a table option has been added to a shell command (I added it for importdir, there was another, but I don't recall specifics) and then this one. Would it be worth creating an issue and checking that we are consistent across all of the commands that operate on a table that they accept this as an option?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub<https://github.com/apache/accumulo/pull/1771#issuecomment-724923053>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHQ4KYKDHGZBYOTEPXYBYSDSPGJIRANCNFSM4TM77QNQ>.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r520262824
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -171,6 +180,11 @@ public Options getOptions() {
"durability to use for insert, should be one of \"none\" \"log\" \"flush\" or \"sync\"");
o.addOption(durabilityOption);
+ tableNameOption =
+ new Option("", "tablename", true, "name of table in which data is being inserted");
+ tableNameOption.setArgName("tablename");
+ o.addOption(tableNameOption);
+
Review comment:
If there's not already a `-t`, I don't think it's a problem. I don't think there's any concern over conflicting with `-ts`. In fact, most commands, use the standard `OptUtil.tableOpt`. See https://github.com/apache/accumulo/blob/main/shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java#L93 and https://github.com/apache/accumulo/blob/main/shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java#L395.
I would recommend doing the same here, if there's no existing `-t`.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r518989512
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -63,81 +64,96 @@ protected long getTimeout(final CommandLine cl) {
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws AccumuloException, AccumuloSecurityException, TableNotFoundException, IOException,
ConstraintViolationException {
- shellState.checkTableState();
- final Mutation m = new Mutation(new Text(cl.getArgs()[0].getBytes(Shell.CHARSET)));
- final Text colf = new Text(cl.getArgs()[1].getBytes(Shell.CHARSET));
- final Text colq = new Text(cl.getArgs()[2].getBytes(Shell.CHARSET));
- final Value val = new Value(cl.getArgs()[3].getBytes(Shell.CHARSET));
-
- if (cl.hasOption(insertOptAuths.getOpt())) {
- final ColumnVisibility le = new ColumnVisibility(cl.getOptionValue(insertOptAuths.getOpt()));
- Shell.log.debug("Authorization label will be set to: {}", le);
-
- if (cl.hasOption(timestampOpt.getOpt()))
- m.put(colf, colq, le, Long.parseLong(cl.getOptionValue(timestampOpt.getOpt())), val);
- else
- m.put(colf, colq, le, val);
- } else if (cl.hasOption(timestampOpt.getOpt()))
- m.put(colf, colq, Long.parseLong(cl.getOptionValue(timestampOpt.getOpt())), val);
- else
- m.put(colf, colq, val);
-
- final BatchWriterConfig cfg =
- new BatchWriterConfig().setMaxMemory(Math.max(m.estimatedMemoryUsed(), 1024))
- .setMaxWriteThreads(1).setTimeout(getTimeout(cl), TimeUnit.MILLISECONDS);
- if (cl.hasOption(durabilityOption.getOpt())) {
- String userDurability = cl.getOptionValue(durabilityOption.getOpt());
- switch (userDurability) {
- case "sync":
- cfg.setDurability(Durability.SYNC);
- break;
- case "flush":
- cfg.setDurability(Durability.FLUSH);
- break;
- case "none":
- cfg.setDurability(Durability.NONE);
- break;
- case "log":
- cfg.setDurability(Durability.NONE);
- break;
- default:
- throw new IllegalArgumentException("Unknown durability: " + userDurability);
- }
- }
- final BatchWriter bw =
- shellState.getAccumuloClient().createBatchWriter(shellState.getTableName(), cfg);
- bw.addMutation(m);
+ String initialTable = null;
try {
- bw.close();
- } catch (MutationsRejectedException e) {
- final ArrayList<String> lines = new ArrayList<>();
- if (!e.getSecurityErrorCodes().isEmpty()) {
- lines.add("\tAuthorization Failures:");
+ if (cl.hasOption(tableNameOption.getOpt())) {
+ initialTable = shellState.getTableName();
+ shellState.setTableName(cl.getOptionValue(tableNameOption.getOpt()));
Review comment:
This command should use the table specified, but it should not update the table state for the entire shell. All you need to do is set a local variable to either the specified table, or default to the current shell state's table, and use that local variable in the line below that creates the batch writer.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#issuecomment-726151829
I created #1776 to address this issue.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r520262824
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -171,6 +180,11 @@ public Options getOptions() {
"durability to use for insert, should be one of \"none\" \"log\" \"flush\" or \"sync\"");
o.addOption(durabilityOption);
+ tableNameOption =
+ new Option("", "tablename", true, "name of table in which data is being inserted");
+ tableNameOption.setArgName("tablename");
+ o.addOption(tableNameOption);
+
Review comment:
If there's not already a `-t`, I don't think it's a problem. I don't think there's any concern over conflicting with `-ts`. In fact, most commands use the standard `OptUtil.tableOpt`. See https://github.com/apache/accumulo/blob/main/shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java#L93 and https://github.com/apache/accumulo/blob/main/shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java#L395.
I would recommend doing the same here, if there's no existing `-t`.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#issuecomment-724960792
@EdColeman I think that would be a reasonable thing to do if you want to pursue it.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r520109854
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -171,6 +180,11 @@ public Options getOptions() {
"durability to use for insert, should be one of \"none\" \"log\" \"flush\" or \"sync\"");
o.addOption(durabilityOption);
+ tableNameOption =
+ new Option("", "tablename", true, "name of table in which data is being inserted");
+ tableNameOption.setArgName("tablename");
+ o.addOption(tableNameOption);
+
Review comment:
What's the reason for not supporting `-t` short name?
----------------------------------------------------------------
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] EdColeman commented on pull request #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#issuecomment-726136025
I do not mind at all!
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 edited a comment on pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#issuecomment-726055015
@EdColeman, I think that would be a good idea. Unless you wish to personally pursue it, I would be willing to take a look since I’m currently reviewing the Accumulo-examples code and coming across various commands that may need the option. Let me know which you would prefer.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r520698476
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -171,6 +171,8 @@ public Options getOptions() {
"durability to use for insert, should be one of \"none\" \"log\" \"flush\" or \"sync\"");
o.addOption(durabilityOption);
+ o.addOption(OptUtil.tableOpt("table in which data is to be inserted"));
Review comment:
No problem. Didn't quite sound right to me, but I was blanking on how to re-word it.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r520693879
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -171,6 +171,8 @@ public Options getOptions() {
"durability to use for insert, should be one of \"none\" \"log\" \"flush\" or \"sync\"");
o.addOption(durabilityOption);
+ o.addOption(OptUtil.tableOpt("table in which data is to be inserted"));
Review comment:
Minor grammatical nit:
```suggestion
o.addOption(OptUtil.tableOpt("table into which data will be inserted"));
```
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r520115394
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -171,6 +180,11 @@ public Options getOptions() {
"durability to use for insert, should be one of \"none\" \"log\" \"flush\" or \"sync\"");
o.addOption(durabilityOption);
+ tableNameOption =
+ new Option("", "tablename", true, "name of table in which data is being inserted");
+ tableNameOption.setArgName("tablename");
+ o.addOption(tableNameOption);
+
Review comment:
I chose not to use the short form to minimize any confusion with the <code>timestamp</code> parameter (-ts). There is also a <code>timeout</code> parameter that does not off a shortened name. If you think it would be nice to include one, I have no problem adding one. Could always use '-n' for 'name' perhaps. Let me know your thoughts.
----------------------------------------------------------------
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 merged pull request #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 merged pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r520115394
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -171,6 +180,11 @@ public Options getOptions() {
"durability to use for insert, should be one of \"none\" \"log\" \"flush\" or \"sync\"");
o.addOption(durabilityOption);
+ tableNameOption =
+ new Option("", "tablename", true, "name of table in which data is being inserted");
+ tableNameOption.setArgName("tablename");
+ o.addOption(tableNameOption);
+
Review comment:
I chose not to use the short form to minimize any confusion with the <code>timestamp</code> parameter (-ts). There is also a <code>timeout<code> parameter that does not off a shortened name. If you think it would be nice to include one, I have no problem adding one. Could always use '-n' for 'name' perhaps. Let me know your thoughts.
----------------------------------------------------------------
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 #1771: Add tablename option to shell insert command.
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #1771:
URL: https://github.com/apache/accumulo/pull/1771#discussion_r519931835
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/InsertCommand.java
##########
@@ -63,81 +64,96 @@ protected long getTimeout(final CommandLine cl) {
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws AccumuloException, AccumuloSecurityException, TableNotFoundException, IOException,
ConstraintViolationException {
- shellState.checkTableState();
- final Mutation m = new Mutation(new Text(cl.getArgs()[0].getBytes(Shell.CHARSET)));
- final Text colf = new Text(cl.getArgs()[1].getBytes(Shell.CHARSET));
- final Text colq = new Text(cl.getArgs()[2].getBytes(Shell.CHARSET));
- final Value val = new Value(cl.getArgs()[3].getBytes(Shell.CHARSET));
-
- if (cl.hasOption(insertOptAuths.getOpt())) {
- final ColumnVisibility le = new ColumnVisibility(cl.getOptionValue(insertOptAuths.getOpt()));
- Shell.log.debug("Authorization label will be set to: {}", le);
-
- if (cl.hasOption(timestampOpt.getOpt()))
- m.put(colf, colq, le, Long.parseLong(cl.getOptionValue(timestampOpt.getOpt())), val);
- else
- m.put(colf, colq, le, val);
- } else if (cl.hasOption(timestampOpt.getOpt()))
- m.put(colf, colq, Long.parseLong(cl.getOptionValue(timestampOpt.getOpt())), val);
- else
- m.put(colf, colq, val);
-
- final BatchWriterConfig cfg =
- new BatchWriterConfig().setMaxMemory(Math.max(m.estimatedMemoryUsed(), 1024))
- .setMaxWriteThreads(1).setTimeout(getTimeout(cl), TimeUnit.MILLISECONDS);
- if (cl.hasOption(durabilityOption.getOpt())) {
- String userDurability = cl.getOptionValue(durabilityOption.getOpt());
- switch (userDurability) {
- case "sync":
- cfg.setDurability(Durability.SYNC);
- break;
- case "flush":
- cfg.setDurability(Durability.FLUSH);
- break;
- case "none":
- cfg.setDurability(Durability.NONE);
- break;
- case "log":
- cfg.setDurability(Durability.NONE);
- break;
- default:
- throw new IllegalArgumentException("Unknown durability: " + userDurability);
- }
- }
- final BatchWriter bw =
- shellState.getAccumuloClient().createBatchWriter(shellState.getTableName(), cfg);
- bw.addMutation(m);
+ String initialTable = null;
try {
- bw.close();
- } catch (MutationsRejectedException e) {
- final ArrayList<String> lines = new ArrayList<>();
- if (!e.getSecurityErrorCodes().isEmpty()) {
- lines.add("\tAuthorization Failures:");
+ if (cl.hasOption(tableNameOption.getOpt())) {
+ initialTable = shellState.getTableName();
+ shellState.setTableName(cl.getOptionValue(tableNameOption.getOpt()));
Review comment:
Thanks for pointing that out @ctubbsii. I've updated the code to use a temp variable and left the shellstate as is.
----------------------------------------------------------------
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