ctubbsii commented on code in PR #3562:
URL: https://github.com/apache/accumulo/pull/3562#discussion_r1248329933
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateNamespaceCommand.java:
##########
@@ -37,33 +37,42 @@
public class CreateNamespaceCommand extends Command {
private Option createNamespaceOptCopyConfig;
+ private Option createNamespaceOptCopyProperties;
@Override
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState)
throws AccumuloException, AccumuloSecurityException,
TableExistsException,
TableNotFoundException, IOException, ClassNotFoundException,
NamespaceExistsException,
NamespaceNotFoundException {
- if (createNamespaceOptCopyConfig == null) {
- getOptions();
- }
+ getOptions();
Review Comment:
I'm not sure what this is doing at all. `getOptions()` will be called by the
framework later. It should also be idempotent and lightweight, so it doesn't
matter if the private members are set or not yet. But by the time this method
is called, they certainly will be... so unless I'm mistaken, this line is
redundant and useless.
I looked around and no other shell commands do this. The only other two that
are "weird" in some way is the GrantCommand and RevokeCommand. In those, their
getOptions() calls super.getOptions(), but just ignores the returned object, it
for some reason. So, they are either depending on side-effects, are pointlessly
calling super.getOptions() that does nothing, or they are broken and not
correctly inheriting options from their super class. Regardless, those are a
separate issue from here.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateNamespaceCommand.java:
##########
@@ -37,33 +37,42 @@
public class CreateNamespaceCommand extends Command {
private Option createNamespaceOptCopyConfig;
+ private Option createNamespaceOptCopyProperties;
@Override
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState)
throws AccumuloException, AccumuloSecurityException,
TableExistsException,
TableNotFoundException, IOException, ClassNotFoundException,
NamespaceExistsException,
NamespaceNotFoundException {
- if (createNamespaceOptCopyConfig == null) {
- getOptions();
- }
+ getOptions();
String namespace = cl.getArgs()[0];
shellState.getAccumuloClient().namespaceOperations().create(namespace);
- // Copy options if flag was set
- Map<String,String> configuration = null;
+ Map<String,String> propsToSet = null;
+
+ // Copy configuration options if flag was set
if (cl.hasOption(createNamespaceOptCopyConfig.getOpt())) {
- String copy = cl.getOptionValue(createNamespaceOptCopyConfig.getOpt());
+ String srcNs = cl.getOptionValue(createNamespaceOptCopyConfig.getOpt());
if
(shellState.getAccumuloClient().namespaceOperations().exists(namespace)) {
- configuration =
shellState.getAccumuloClient().namespaceOperations().getConfiguration(copy);
+ propsToSet =
shellState.getAccumuloClient().namespaceOperations().getConfiguration(srcNs);
}
}
- if (configuration != null) {
- final Map<String,String> config = configuration;
+
+ if (cl.hasOption(createNamespaceOptCopyProperties.getOpt())) {
+ String srcNs =
cl.getOptionValue(createNamespaceOptCopyProperties.getOpt());
+ if
(shellState.getAccumuloClient().namespaceOperations().exists(namespace)) {
+ propsToSet =
+
shellState.getAccumuloClient().namespaceOperations().getNamespaceProperties(srcNs);
+ }
+ }
+
+ if (propsToSet != null) {
+ final Map<String,String> props = propsToSet;
shellState.getAccumuloClient().namespaceOperations().modifyProperties(namespace,
Review Comment:
It'd be nice if we didn't have to mutate the properties, but could set them
at creation time, similar to NewTableConfiguration. This could fail after the
namespace is created, but without modifying the properties.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateNamespaceCommand.java:
##########
@@ -87,11 +96,13 @@ public Options getOptions() {
createNamespaceOptCopyConfig =
new Option("cc", "copy-config", true, "namespace to copy configuration
from");
+ createNamespaceOptCopyProperties = new Option("cp", "copy-properties",
true,
+ "namespace to copy properties from (only namespace properties are
copied)");
Review Comment:
This is so similarly named. Although the naming similarity affects the Java
API also, the Java API has the benefit of a decent Javadoc. This could use a
better name to avoid getting too verbose help documentation.
For example, instead of creating a separate property (and allowing them both
to be used, which could get confusing), just add a
`--copy-config-exclude-parent` or just `--exclude-parent` that's used in
conjunction with `--copy-config`.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateNamespaceCommand.java:
##########
@@ -37,33 +37,42 @@
public class CreateNamespaceCommand extends Command {
private Option createNamespaceOptCopyConfig;
+ private Option createNamespaceOptCopyProperties;
@Override
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState)
throws AccumuloException, AccumuloSecurityException,
TableExistsException,
TableNotFoundException, IOException, ClassNotFoundException,
NamespaceExistsException,
NamespaceNotFoundException {
- if (createNamespaceOptCopyConfig == null) {
- getOptions();
- }
+ getOptions();
String namespace = cl.getArgs()[0];
shellState.getAccumuloClient().namespaceOperations().create(namespace);
- // Copy options if flag was set
- Map<String,String> configuration = null;
+ Map<String,String> propsToSet = null;
+
+ // Copy configuration options if flag was set
if (cl.hasOption(createNamespaceOptCopyConfig.getOpt())) {
- String copy = cl.getOptionValue(createNamespaceOptCopyConfig.getOpt());
+ String srcNs = cl.getOptionValue(createNamespaceOptCopyConfig.getOpt());
if
(shellState.getAccumuloClient().namespaceOperations().exists(namespace)) {
- configuration =
shellState.getAccumuloClient().namespaceOperations().getConfiguration(copy);
+ propsToSet =
shellState.getAccumuloClient().namespaceOperations().getConfiguration(srcNs);
}
}
- if (configuration != null) {
- final Map<String,String> config = configuration;
+
+ if (cl.hasOption(createNamespaceOptCopyProperties.getOpt())) {
+ String srcNs =
cl.getOptionValue(createNamespaceOptCopyProperties.getOpt());
+ if
(shellState.getAccumuloClient().namespaceOperations().exists(namespace)) {
+ propsToSet =
+
shellState.getAccumuloClient().namespaceOperations().getNamespaceProperties(srcNs);
+ }
+ }
Review Comment:
I didn't realize we already had an API that got only the namespace
properties. I thought the new API showed the effective properties, just like
the old API.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateNamespaceCommand.java:
##########
@@ -37,33 +37,42 @@
public class CreateNamespaceCommand extends Command {
private Option createNamespaceOptCopyConfig;
+ private Option createNamespaceOptCopyProperties;
@Override
public int execute(final String fullCommand, final CommandLine cl, final
Shell shellState)
throws AccumuloException, AccumuloSecurityException,
TableExistsException,
TableNotFoundException, IOException, ClassNotFoundException,
NamespaceExistsException,
NamespaceNotFoundException {
- if (createNamespaceOptCopyConfig == null) {
- getOptions();
- }
+ getOptions();
String namespace = cl.getArgs()[0];
shellState.getAccumuloClient().namespaceOperations().create(namespace);
- // Copy options if flag was set
- Map<String,String> configuration = null;
+ Map<String,String> propsToSet = null;
+
+ // Copy configuration options if flag was set
if (cl.hasOption(createNamespaceOptCopyConfig.getOpt())) {
- String copy = cl.getOptionValue(createNamespaceOptCopyConfig.getOpt());
+ String srcNs = cl.getOptionValue(createNamespaceOptCopyConfig.getOpt());
if
(shellState.getAccumuloClient().namespaceOperations().exists(namespace)) {
- configuration =
shellState.getAccumuloClient().namespaceOperations().getConfiguration(copy);
+ propsToSet =
shellState.getAccumuloClient().namespaceOperations().getConfiguration(srcNs);
}
}
- if (configuration != null) {
- final Map<String,String> config = configuration;
+
+ if (cl.hasOption(createNamespaceOptCopyProperties.getOpt())) {
+ String srcNs =
cl.getOptionValue(createNamespaceOptCopyProperties.getOpt());
+ if
(shellState.getAccumuloClient().namespaceOperations().exists(namespace)) {
Review Comment:
This existence check will hide the errors if the table doesn't exist. That
looks like it's a pre-existing problem. If the user specifies to copy from a
namespace that doesn't exist (probably most often from a typo), I'd expect a
NamespaceNotFoundException, not silent success without copying anything. Even
though it's a pre-existing problem, it shouldn't be carried forward into this
new implementation, and the old implementation should be fixed, too.
--
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]
