You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "rsingh433 (via GitHub)" <gi...@apache.org> on 2023/12/20 18:59:40 UTC
[PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
rsingh433 opened a new pull request, #4096:
URL: https://github.com/apache/accumulo/pull/4096
(no comment)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1442257678
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java:
##########
@@ -566,6 +568,18 @@ public void propFileNotFoundTest() throws IOException {
"FileNotFoundException: " + fileName + "(No such file or directory)");
}
+ @Test
Review Comment:
You could be using
```
@TempDir
private static File tempDir;
```
See ServiceLockIT (that's one example I found where its used)
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1439782751
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -424,19 +474,21 @@ public Options getOptions() {
outputFileOpt = new Option("o", "output", true, "local file to write the scan output to");
namespaceOpt = new Option(ShellOptions.namespaceOption, "namespace", true,
"namespace to display/set/delete properties for");
-
+ propFileOpt = new Option("p", "propFileOpt", true, "file containing properties to set");
Review Comment:
```suggestion
propFileOpt = new Option("p", "propFile", true, "file containing properties to set");
```
Only the variable name needed to change. The full flag the user passes in should remain `propFile`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #4096:
URL: https://github.com/apache/accumulo/pull/4096
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1436572946
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -424,19 +492,21 @@ public Options getOptions() {
outputFileOpt = new Option("o", "output", true, "local file to write the scan output to");
namespaceOpt = new Option(ShellOptions.namespaceOption, "namespace", true,
"namespace to display/set/delete properties for");
-
+ fileOpt = new Option("fs", "file", true, "file containing properties to set");
Review Comment:
I assume the `f` is used for filter? `fs` (without context) seems like it should be for file system. Would `p` and `propFile` 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#issuecomment-1869803876
The build failed with a dependency problem
```
[ERROR] Used undeclared dependencies found:
[ERROR] org.apache.commons:commons-configuration2:jar:2.9.0:compile
```
The dependency needs to be added to the ./shell/pom.xml file. The version is declared in the parent pom, so:
```
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-configuration2</artifactId>
</dependency>
```
should be sufficient,
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1446594161
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java:
##########
@@ -557,6 +560,42 @@ public void execFileTest() throws IOException {
assertGoodExit("Unknown command", false);
}
+ @TempDir
+ private static File tempDir;
+
+ @Test
+ public void propFileNotFoundTest() throws IOException {
+ String fileName = new File(tempDir, "propFile.shellit").getAbsolutePath();
+
+ Shell.log.debug("Starting prop file not found test --------------------------");
+ exec("config --propFile " + fileName, false,
+ "FileNotFoundException: " + fileName + "(No such file or directory)");
+ }
+
+ @Test
+ public void setpropsViaFile() throws Exception {
+
+ File file = File.createTempFile("propFile", ".conf",
+ new File("src/main/resources/org/apache/accumulo/test/"));
+ PrintWriter writer = new PrintWriter(file.getAbsolutePath());
+ writer.println(COMPACTION_WARN_TIME.getKey() + "=11m");
+ writer.close();
+ exec("config --propFile " + file.getAbsolutePath(), true);
+ file.deleteOnExit();
+ }
+
+ @Test
+ public void invalidPropFileTest() throws Exception {
+ File file = File.createTempFile("invalidPropFile", ".conf",
+ new File("src/main/resources/org/apache/accumulo/test/"));
+ PrintWriter writer = new PrintWriter(file.getAbsolutePath());
+ writer.println("this is not a valid property file");
+ writer.close();
+ exec("config --propFile " + file.getAbsolutePath(), false,
+ "InvalidPropertyFile: " + file.getAbsolutePath());
+ file.deleteOnExit();
Review Comment:
```suggestion
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1446594385
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java:
##########
@@ -557,6 +560,42 @@ public void execFileTest() throws IOException {
assertGoodExit("Unknown command", false);
}
+ @TempDir
+ private static File tempDir;
+
+ @Test
+ public void propFileNotFoundTest() throws IOException {
+ String fileName = new File(tempDir, "propFile.shellit").getAbsolutePath();
+
+ Shell.log.debug("Starting prop file not found test --------------------------");
+ exec("config --propFile " + fileName, false,
+ "FileNotFoundException: " + fileName + "(No such file or directory)");
+ }
+
+ @Test
+ public void setpropsViaFile() throws Exception {
+
+ File file = File.createTempFile("propFile", ".conf",
+ new File("src/main/resources/org/apache/accumulo/test/"));
+ PrintWriter writer = new PrintWriter(file.getAbsolutePath());
+ writer.println(COMPACTION_WARN_TIME.getKey() + "=11m");
+ writer.close();
+ exec("config --propFile " + file.getAbsolutePath(), true);
+ file.deleteOnExit();
+ }
+
+ @Test
+ public void invalidPropFileTest() throws Exception {
+ File file = File.createTempFile("invalidPropFile", ".conf",
+ new File("src/main/resources/org/apache/accumulo/test/"));
Review Comment:
Same comment, update this to use tempDir
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "rsingh433 (via GitHub)" <gi...@apache.org>.
rsingh433 commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1436618300
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java:
##########
@@ -424,19 +492,21 @@ public Options getOptions() {
outputFileOpt = new Option("o", "output", true, "local file to write the scan output to");
namespaceOpt = new Option(ShellOptions.namespaceOption, "namespace", true,
"namespace to display/set/delete properties for");
-
+ fileOpt = new Option("fs", "file", true, "file containing properties to set");
Review Comment:
I can definitely switch it to p for propFile.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1437802314
##########
test/pom.xml:
##########
@@ -127,10 +127,6 @@
<groupId>org.apache.accumulo</groupId>
<artifactId>accumulo-tserver</artifactId>
</dependency>
- <dependency>
Review Comment:
@rsingh433 You removed the duplicate dependency entry on line 90, but also removed this original entry.
Once you restore this entry, I believe your build should pass.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1446593961
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java:
##########
@@ -557,6 +560,42 @@ public void execFileTest() throws IOException {
assertGoodExit("Unknown command", false);
}
+ @TempDir
+ private static File tempDir;
+
+ @Test
+ public void propFileNotFoundTest() throws IOException {
+ String fileName = new File(tempDir, "propFile.shellit").getAbsolutePath();
+
+ Shell.log.debug("Starting prop file not found test --------------------------");
+ exec("config --propFile " + fileName, false,
+ "FileNotFoundException: " + fileName + "(No such file or directory)");
+ }
+
+ @Test
+ public void setpropsViaFile() throws Exception {
+
+ File file = File.createTempFile("propFile", ".conf",
+ new File("src/main/resources/org/apache/accumulo/test/"));
+ PrintWriter writer = new PrintWriter(file.getAbsolutePath());
+ writer.println(COMPACTION_WARN_TIME.getKey() + "=11m");
+ writer.close();
+ exec("config --propFile " + file.getAbsolutePath(), true);
+ file.deleteOnExit();
Review Comment:
Once this test uses TempDir this line is no longer needed as tempDir is cleaned up by JUnit after a test run.
```suggestion
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added modifyPropertiesFromFile Method in ConfigCommand [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4096:
URL: https://github.com/apache/accumulo/pull/4096#discussion_r1446592718
##########
test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java:
##########
@@ -557,6 +560,42 @@ public void execFileTest() throws IOException {
assertGoodExit("Unknown command", false);
}
+ @TempDir
+ private static File tempDir;
+
+ @Test
+ public void propFileNotFoundTest() throws IOException {
+ String fileName = new File(tempDir, "propFile.shellit").getAbsolutePath();
+
+ Shell.log.debug("Starting prop file not found test --------------------------");
+ exec("config --propFile " + fileName, false,
+ "FileNotFoundException: " + fileName + "(No such file or directory)");
+ }
+
+ @Test
+ public void setpropsViaFile() throws Exception {
+
+ File file = File.createTempFile("propFile", ".conf",
+ new File("src/main/resources/org/apache/accumulo/test/"));
Review Comment:
These file creations should also be updated to use the `tempDir`.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org