cshannon commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r996299622
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java:
##########
@@ -134,22 +135,24 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
shellState.setTableName(tableName); // switch shell to new table context
if (cl.hasOption(createTableNoDefaultIters.getOpt())) {
- for (String key :
IteratorConfigUtil.generateInitialTableProperties(true).keySet()) {
-
shellState.getAccumuloClient().tableOperations().removeProperty(tableName, key);
- }
+ Set<String> initialProps =
IteratorConfigUtil.generateInitialTableProperties(true).keySet();
+
shellState.getAccumuloClient().tableOperations().modifyProperties(tableName,
+ properties -> initialProps.forEach(properties::remove));
}
// Copy options if flag was set
if (cl.hasOption(createTableOptCopyConfig.getOpt())) {
if (shellState.getAccumuloClient().tableOperations().exists(tableName)) {
final Map<String,String> configuration =
shellState.getAccumuloClient().tableOperations()
.getConfiguration(cl.getOptionValue(createTableOptCopyConfig.getOpt()));
- for (Entry<String,String> entry : configuration.entrySet()) {
- if (Property.isValidTablePropertyKey(entry.getKey())) {
-
shellState.getAccumuloClient().tableOperations().setProperty(tableName,
entry.getKey(),
- entry.getValue());
- }
- }
+
+ Map<String,
+ String> propsToAdd = configuration.entrySet().stream()
+ .filter(entry ->
Property.isValidTablePropertyKey(entry.getKey()))
+ .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
+
+
shellState.getAccumuloClient().tableOperations().modifyProperties(tableName,
+ properties -> properties.putAll(propsToAdd));
Review Comment:
This change would make it so we only need to iterate once over the
properties vs having to iterate over the map twice (once to filter into a new
map and then again when putAll() is called)
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/TableOperationsHelperTest.java:
##########
@@ -133,22 +133,18 @@ public void flush(String tableName, Text start, Text end,
boolean wait) {}
@Override
public void setProperty(String tableName, String property, String value) {
- if (!settings.containsKey(tableName))
- settings.put(tableName, new TreeMap<>());
+ settings.putIfAbsent(tableName, new TreeMap<>());
settings.get(tableName).put(property, value);
}
@Override
public Map<String,String> modifyProperties(String tableName,
Consumer<Map<String,String>> mapMutator)
throws IllegalArgumentException, ConcurrentModificationException {
+ settings.putIfAbsent(tableName, new TreeMap<>());
Review Comment:
```suggestion
settings.computeIfAbsent(tableName, k -> new TreeMap<>());
```
##########
core/src/test/java/org/apache/accumulo/core/clientImpl/TableOperationsHelperTest.java:
##########
@@ -133,22 +133,18 @@ public void flush(String tableName, Text start, Text end,
boolean wait) {}
@Override
public void setProperty(String tableName, String property, String value) {
- if (!settings.containsKey(tableName))
- settings.put(tableName, new TreeMap<>());
+ settings.putIfAbsent(tableName, new TreeMap<>());
settings.get(tableName).put(property, value);
}
@Override
public Map<String,String> modifyProperties(String tableName,
Consumer<Map<String,String>> mapMutator)
throws IllegalArgumentException, ConcurrentModificationException {
+ settings.putIfAbsent(tableName, new TreeMap<>());
Review Comment:
Small nit but if you use computeIfAbsent here then you save on the
allocation each time of a new TreeMap that you don't need unless the value is
absent so you reduce garbage collection since it's lazy allocated. Not a big
deal since this is just a unit test but in general useful.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java:
##########
@@ -134,22 +135,24 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
shellState.setTableName(tableName); // switch shell to new table context
if (cl.hasOption(createTableNoDefaultIters.getOpt())) {
- for (String key :
IteratorConfigUtil.generateInitialTableProperties(true).keySet()) {
-
shellState.getAccumuloClient().tableOperations().removeProperty(tableName, key);
- }
+ Set<String> initialProps =
IteratorConfigUtil.generateInitialTableProperties(true).keySet();
+
shellState.getAccumuloClient().tableOperations().modifyProperties(tableName,
+ properties -> initialProps.forEach(properties::remove));
}
// Copy options if flag was set
if (cl.hasOption(createTableOptCopyConfig.getOpt())) {
if (shellState.getAccumuloClient().tableOperations().exists(tableName)) {
final Map<String,String> configuration =
shellState.getAccumuloClient().tableOperations()
.getConfiguration(cl.getOptionValue(createTableOptCopyConfig.getOpt()));
- for (Entry<String,String> entry : configuration.entrySet()) {
- if (Property.isValidTablePropertyKey(entry.getKey())) {
-
shellState.getAccumuloClient().tableOperations().setProperty(tableName,
entry.getKey(),
- entry.getValue());
- }
- }
+
+ Map<String,
+ String> propsToAdd = configuration.entrySet().stream()
+ .filter(entry ->
Property.isValidTablePropertyKey(entry.getKey()))
+ .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
+
+
shellState.getAccumuloClient().tableOperations().modifyProperties(tableName,
+ properties -> properties.putAll(propsToAdd));
Review Comment:
```suggestion
shellState.getAccumuloClient().tableOperations().modifyProperties(tableName,
properties -> configuration.entrySet().stream()
.filter(entry ->
Property.isValidTablePropertyKey(entry.getKey()))
.forEach(entry -> properties.put(entry.getKey(),
entry.getValue())));
```
--
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]
