ddanielr commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1438269747
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -81,6 +89,12 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
boolean force = cl.hasOption(forceOpt);
+ String filename = cl.getOptionValue("p");
Review Comment:
```suggestion
String filename = cl.getOptionValue(propFile.getOpt());
```
Use the option you created instead of a hardcoded value.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -368,6 +382,69 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
return 0;
}
+ // New method for property modification logic
+ private void modifyPropertiesFromFile(CommandLine cl, Shell shellState,
String filename,
+ boolean force) throws AccumuloException, AccumuloSecurityException,
TableNotFoundException,
+ IOException, NamespaceNotFoundException {
+ // Read properties from the file
+ PropertiesConfiguration fileProperties = readPropertiesFromFile(filename);
+
+ // Convert PropertiesConfiguration to Map<String, String>
+ Map<String,String> propertiesMap = new HashMap<>();
+ Iterator<String> keysIterator = fileProperties.getKeys();
+ while (keysIterator.hasNext()) {
+ // Iterate through the keys in the PropertiesConfiguration
+ String key = keysIterator.next();
+ // Get the value associated with the key
+ String value = fileProperties.getString(key);
+ // Add the key-value pair to the propertiesMap
+ propertiesMap.put(key, value);
Review Comment:
```suggestion
fileProperties.getKeys().forEachRemaining(key -> {
propertiesMap.put(key, fileProperties.getString(key));
});
```
Optional Change: You could clean this code up a bit by using a lambda.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -368,6 +382,69 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
return 0;
}
+ // New method for property modification logic
+ private void modifyPropertiesFromFile(CommandLine cl, Shell shellState,
String filename,
+ boolean force) throws AccumuloException, AccumuloSecurityException,
TableNotFoundException,
+ IOException, NamespaceNotFoundException {
+ // Read properties from the file
+ PropertiesConfiguration fileProperties = readPropertiesFromFile(filename);
+
+ // Convert PropertiesConfiguration to Map<String, String>
+ Map<String,String> propertiesMap = new HashMap<>();
+ Iterator<String> keysIterator = fileProperties.getKeys();
+ while (keysIterator.hasNext()) {
+ // Iterate through the keys in the PropertiesConfiguration
+ String key = keysIterator.next();
+ // Get the value associated with the key
+ String value = fileProperties.getString(key);
+ // Add the key-value pair to the propertiesMap
+ propertiesMap.put(key, value);
+ }
+
+ // Create a consumer for property modification
+ Consumer<Map<String,String>> propertyModifier = currProps -> {
+ // Update currProps
+ currProps.putAll(propertiesMap);
+ };
+
+ // Modify properties based on other options
+ if (cl.hasOption("t")) {
+ // Modify table properties
+
shellState.getAccumuloClient().tableOperations().modifyProperties(cl.getOptionValue("t"),
+ propertyModifier);
+ } else if (cl.hasOption("n")) {
+ // Modify namespace properties
+
shellState.getAccumuloClient().namespaceOperations().modifyProperties(cl.getOptionValue("n"),
+ propertyModifier);
+ } else {
+ // Modify system properties
+ // Obtain the InstanceOperations object for the Accumulo client
associated with the shell
+ // state
+ InstanceOperations instanceOperations =
shellState.getAccumuloClient().instanceOperations();
+
+ // Iterate over each entry in the propertiesMap (which represents
properties to be modified)
+ for (Map.Entry<String,String> entry : propertiesMap.entrySet()) {
+ // Get the key and value of the current property entry
+ String key = entry.getKey();
+ String value = entry.getValue();
+
+ // Set the system property identified by the key to the specified value
+ instanceOperations.setProperty(key, value);
+ }
+ }
+ }
+
+ // New method to read properties from a file
+ private PropertiesConfiguration readPropertiesFromFile(String filename)
throws IOException {
+ var config = new PropertiesConfiguration();
+ try (FileReader out = new FileReader(filename, UTF_8)) {
+ config.read(out);
+ } catch (ConfigurationException e) {
+ throw new IOException(e);
Review Comment:
Instead of changing this to an IOException you could catch it and send an
error message.
```suggestion
} catch (ConfigurationException e) {
Shell.log.error(
"Property file {} contains invalid configuration. Please
verify file format", filename, e);
```
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -368,6 +382,69 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
return 0;
}
+ // New method for property modification logic
+ private void modifyPropertiesFromFile(CommandLine cl, Shell shellState,
String filename,
+ boolean force) throws AccumuloException, AccumuloSecurityException,
TableNotFoundException,
Review Comment:
The `force` boolean isn't used in this method so it can be removed.
```suggestion
private void modifyPropertiesFromFile(CommandLine cl, Shell shellState,
String filename) throws
AccumuloException, AccumuloSecurityException, TableNotFoundException,
```
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -368,6 +382,69 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
return 0;
}
+ // New method for property modification logic
+ private void modifyPropertiesFromFile(CommandLine cl, Shell shellState,
String filename,
+ boolean force) throws AccumuloException, AccumuloSecurityException,
TableNotFoundException,
+ IOException, NamespaceNotFoundException {
+ // Read properties from the file
+ PropertiesConfiguration fileProperties = readPropertiesFromFile(filename);
+
+ // Convert PropertiesConfiguration to Map<String, String>
+ Map<String,String> propertiesMap = new HashMap<>();
+ Iterator<String> keysIterator = fileProperties.getKeys();
+ while (keysIterator.hasNext()) {
+ // Iterate through the keys in the PropertiesConfiguration
+ String key = keysIterator.next();
+ // Get the value associated with the key
+ String value = fileProperties.getString(key);
+ // Add the key-value pair to the propertiesMap
+ propertiesMap.put(key, value);
+ }
+
+ // Create a consumer for property modification
+ Consumer<Map<String,String>> propertyModifier = currProps -> {
+ // Update currProps
+ currProps.putAll(propertiesMap);
+ };
+
+ // Modify properties based on other options
+ if (cl.hasOption("t")) {
+ // Modify table properties
+
shellState.getAccumuloClient().tableOperations().modifyProperties(cl.getOptionValue("t"),
+ propertyModifier);
+ } else if (cl.hasOption("n")) {
+ // Modify namespace properties
+
shellState.getAccumuloClient().namespaceOperations().modifyProperties(cl.getOptionValue("n"),
+ propertyModifier);
Review Comment:
```suggestion
if (cl.hasOption(tableOpt.getOpt())) {
// Modify table properties
shellState.getAccumuloClient().tableOperations().modifyProperties(cl.getOptionValue(tableOpt.getOpt()),
propertyModifier);
} else if (cl.hasOption(namespaceOpt.getOpt())) {
// Modify namespace properties
shellState.getAccumuloClient().namespaceOperations().modifyProperties(cl.getOptionValue(namespaceOpt.getOpt()),
propertyModifier);
```
Reference the opt variables instead of using hard coded strings.
This way, opt string values are only set in the `getOptions()` method and we
can avoid accidental naming collisions in the code.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -47,14 +53,16 @@
import org.apache.commons.cli.Option;
import org.apache.commons.cli.OptionGroup;
import org.apache.commons.cli.Options;
+import org.apache.commons.configuration2.PropertiesConfiguration;
+import org.apache.commons.configuration2.ex.ConfigurationException;
import org.apache.commons.lang3.StringUtils;
import org.jline.reader.LineReader;
import com.google.common.collect.ImmutableSortedMap;
public class ConfigCommand extends Command {
private Option tableOpt, deleteOpt, setOpt, forceOpt, filterOpt,
filterWithValuesOpt,
- disablePaginationOpt, outputFileOpt, namespaceOpt;
+ disablePaginationOpt, outputFileOpt, namespaceOpt, propFile;
Review Comment:
```suggestion
disablePaginationOpt, outputFileOpt, namespaceOpt, propFileOpt;
```
Rename this to follow the naming convention of the other Option variables.
This will avoid future confusion if a variable is the actual file or the
command line option.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -368,6 +382,69 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
return 0;
}
+ // New method for property modification logic
+ private void modifyPropertiesFromFile(CommandLine cl, Shell shellState,
String filename,
+ boolean force) throws AccumuloException, AccumuloSecurityException,
TableNotFoundException,
+ IOException, NamespaceNotFoundException {
+ // Read properties from the file
+ PropertiesConfiguration fileProperties = readPropertiesFromFile(filename);
+
+ // Convert PropertiesConfiguration to Map<String, String>
+ Map<String,String> propertiesMap = new HashMap<>();
+ Iterator<String> keysIterator = fileProperties.getKeys();
+ while (keysIterator.hasNext()) {
+ // Iterate through the keys in the PropertiesConfiguration
+ String key = keysIterator.next();
+ // Get the value associated with the key
+ String value = fileProperties.getString(key);
+ // Add the key-value pair to the propertiesMap
+ propertiesMap.put(key, value);
+ }
+
+ // Create a consumer for property modification
+ Consumer<Map<String,String>> propertyModifier = currProps -> {
+ // Update currProps
+ currProps.putAll(propertiesMap);
+ };
+
+ // Modify properties based on other options
+ if (cl.hasOption("t")) {
+ // Modify table properties
+
shellState.getAccumuloClient().tableOperations().modifyProperties(cl.getOptionValue("t"),
+ propertyModifier);
+ } else if (cl.hasOption("n")) {
+ // Modify namespace properties
+
shellState.getAccumuloClient().namespaceOperations().modifyProperties(cl.getOptionValue("n"),
+ propertyModifier);
+ } else {
+ // Modify system properties
+ // Obtain the InstanceOperations object for the Accumulo client
associated with the shell
+ // state
+ InstanceOperations instanceOperations =
shellState.getAccumuloClient().instanceOperations();
+
+ // Iterate over each entry in the propertiesMap (which represents
properties to be modified)
+ for (Map.Entry<String,String> entry : propertiesMap.entrySet()) {
+ // Get the key and value of the current property entry
+ String key = entry.getKey();
+ String value = entry.getValue();
+
+ // Set the system property identified by the key to the specified value
+ instanceOperations.setProperty(key, value);
+ }
Review Comment:
Is there a specific reason you are using the `setProperty` API for setting
the system properties instead of `modifyProperties`?
```suggestion
shellState.getAccumuloClient().instanceOperations().modifyProperties(propertyModifier);
```
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -81,6 +89,12 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
boolean force = cl.hasOption(forceOpt);
+ String filename = cl.getOptionValue("p");
+ if (filename != null) {
+ // Call the method to modify properties
+ modifyPropertiesFromFile(cl, shellState, filename, force);
+ }
+
Review Comment:
I would move this section down and insert it after line 92 to take advantage
of the namespace and table checks.
```
final String tableName = cl.getOptionValue(tableOpt.getOpt());
if (tableName != null &&
!shellState.getAccumuloClient().tableOperations().exists(tableName)) {
throw new TableNotFoundException(null, tableName, null);
}
final String namespace = cl.getOptionValue(namespaceOpt.getOpt());
if (namespace != null
&&
!shellState.getAccumuloClient().namespaceOperations().exists(namespace)) {
throw new NamespaceNotFoundException(null, namespace, null);
}
String filename = cl.getOptionValue("p");
if (filename != null) {
// Call the method to modify properties
modifyPropertiesFromFile(cl, shellState, filename, force);
}
```
--
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]
