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 2022/09/26 19:50:17 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #2965: Use TableOperations.modifyProperties in more places
DomGarguilo opened a new pull request, #2965:
URL: https://github.com/apache/accumulo/pull/2965
The changes in this PR make use of the new modify properties method introduced in #2799
--
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
[GitHub] [accumulo] DomGarguilo commented on pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#issuecomment-1280940701
Your suggestions/comments should be addressed @cshannon, if you would like to take another look.
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r997091652
##########
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:
Oh cool, I did not know that, thanks!
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r997094548
##########
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:
Do you think I should refactor all of the spots that are similar to this to reduce the number of times we iterate?
--
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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r980470298
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -56,10 +57,12 @@ public void attachIterator(String tableName, IteratorSetting setting,
for (IteratorScope scope : scopes) {
String root = String.format("%s%s.%s", Property.TABLE_ITERATOR_PREFIX,
scope.name().toLowerCase(), setting.getName());
- for (Entry<String,String> prop : setting.getOptions().entrySet()) {
- this.setProperty(tableName, root + ".opt." + prop.getKey(), prop.getValue());
- }
- this.setProperty(tableName, root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ Map<String,String> propsToAdd = setting.getOptions().entrySet().stream()
+ .collect(Collectors.toMap(prop -> root + ".opt." + prop.getKey(), Entry::getValue));
+ propsToAdd.put(root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ this.modifyProperties(tableName, props -> props.putAll(propsToAdd));
Review Comment:
If no one else is working on it or opposed to it, I would like to try creating a PR for the new modifyProperties method to make it retry automatically instead of throwing the ConcurrentModificationException. If that was done, then this PR could stay as is and not have to handle that condition.
--
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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r980467194
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -56,10 +57,12 @@ public void attachIterator(String tableName, IteratorSetting setting,
for (IteratorScope scope : scopes) {
String root = String.format("%s%s.%s", Property.TABLE_ITERATOR_PREFIX,
scope.name().toLowerCase(), setting.getName());
- for (Entry<String,String> prop : setting.getOptions().entrySet()) {
- this.setProperty(tableName, root + ".opt." + prop.getKey(), prop.getValue());
- }
- this.setProperty(tableName, root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ Map<String,String> propsToAdd = setting.getOptions().entrySet().stream()
+ .collect(Collectors.toMap(prop -> root + ".opt." + prop.getKey(), Entry::getValue));
+ propsToAdd.put(root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ this.modifyProperties(tableName, props -> props.putAll(propsToAdd));
Review Comment:
ConcurrentModificationException is not being handled. The previous code would set the props and never throw this. Now a user could spuriously get this exception.
I didn't realize that ConcurrentModificationException was a run time exception when looking at #2799. I don't think that is good user experience for a spurious exception that user can not take any action to avoid and must handle. Since its a run time exception when a developer is writing code they will most likely be unaware of it and it will most likely not be thrown in their testing.
--
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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r981221098
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -56,10 +57,12 @@ public void attachIterator(String tableName, IteratorSetting setting,
for (IteratorScope scope : scopes) {
String root = String.format("%s%s.%s", Property.TABLE_ITERATOR_PREFIX,
scope.name().toLowerCase(), setting.getName());
- for (Entry<String,String> prop : setting.getOptions().entrySet()) {
- this.setProperty(tableName, root + ".opt." + prop.getKey(), prop.getValue());
- }
- this.setProperty(tableName, root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ Map<String,String> propsToAdd = setting.getOptions().entrySet().stream()
+ .collect(Collectors.toMap(prop -> root + ".opt." + prop.getKey(), Entry::getValue));
+ propsToAdd.put(root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ this.modifyProperties(tableName, props -> props.putAll(propsToAdd));
Review Comment:
When I run the new IT added in #2697 I see evidence of many retries in the logs, so the threads are bumping into each other and retrying.
--
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
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r980575829
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -56,10 +57,12 @@ public void attachIterator(String tableName, IteratorSetting setting,
for (IteratorScope scope : scopes) {
String root = String.format("%s%s.%s", Property.TABLE_ITERATOR_PREFIX,
scope.name().toLowerCase(), setting.getName());
- for (Entry<String,String> prop : setting.getOptions().entrySet()) {
- this.setProperty(tableName, root + ".opt." + prop.getKey(), prop.getValue());
- }
- this.setProperty(tableName, root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ Map<String,String> propsToAdd = setting.getOptions().entrySet().stream()
+ .collect(Collectors.toMap(prop -> root + ".opt." + prop.getKey(), Entry::getValue));
+ propsToAdd.put(root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ this.modifyProperties(tableName, props -> props.putAll(propsToAdd));
Review Comment:
Another option could be to catch and turn the ConcurrentModificationException into a CheckedException so that callers are forced to handle it. If two (or more) separate processes are trying to set the same properties, a retry could effectively allow the last one to always "win". If the catcher just wraps the CheckedException in a retry that's what would happen anyway - but it would be done explicitly.
If the retry allows different, non-overlapping changes, that might be the "best" case. P1 sets A, B, and P2 sets C, D then the final result should be A, B, C and D. The "worst" case would be if the final values were A,B or C,D
If the sets are overlapping then the result would be non-determinant P1 sets A, deletes B and P2 sets B and C, then B could be set or deleted. Changing the same property should be much less frequent than modifying non-overlapping sets.
Currently we do not guarantee that a process will see a given set of properties anyway - say for example a scan - it will execute with whatever it happens to find - if you set a property you need to allow it to propagate to the tservers, but something could change it before the scan actually starts and we do not specify otherwise.
--
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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r981221098
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -56,10 +57,12 @@ public void attachIterator(String tableName, IteratorSetting setting,
for (IteratorScope scope : scopes) {
String root = String.format("%s%s.%s", Property.TABLE_ITERATOR_PREFIX,
scope.name().toLowerCase(), setting.getName());
- for (Entry<String,String> prop : setting.getOptions().entrySet()) {
- this.setProperty(tableName, root + ".opt." + prop.getKey(), prop.getValue());
- }
- this.setProperty(tableName, root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ Map<String,String> propsToAdd = setting.getOptions().entrySet().stream()
+ .collect(Collectors.toMap(prop -> root + ".opt." + prop.getKey(), Entry::getValue));
+ propsToAdd.put(root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ this.modifyProperties(tableName, props -> props.putAll(propsToAdd));
Review Comment:
When I run the new IT added in #2967 I see evidence of many retries in the logs, so the threads are bumping into each other and retrying.
--
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
[GitHub] [accumulo] cshannon commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon merged pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
cshannon merged PR #2965:
URL: https://github.com/apache/accumulo/pull/2965
--
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
[GitHub] [accumulo] keith-turner commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r981216116
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -56,10 +57,12 @@ public void attachIterator(String tableName, IteratorSetting setting,
for (IteratorScope scope : scopes) {
String root = String.format("%s%s.%s", Property.TABLE_ITERATOR_PREFIX,
scope.name().toLowerCase(), setting.getName());
- for (Entry<String,String> prop : setting.getOptions().entrySet()) {
- this.setProperty(tableName, root + ".opt." + prop.getKey(), prop.getValue());
- }
- this.setProperty(tableName, root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ Map<String,String> propsToAdd = setting.getOptions().entrySet().stream()
+ .collect(Collectors.toMap(prop -> root + ".opt." + prop.getKey(), Entry::getValue));
+ propsToAdd.put(root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ this.modifyProperties(tableName, props -> props.putAll(propsToAdd));
Review Comment:
> Another option could be to catch and turn the ConcurrentModificationException into a CheckedException so that callers are forced to handle it.
I am not a fan of this personally. Looking at this PR, each call would have to handle the same case. So that would lead to creating an internal static utility method that does the retry.
> If the retry allows different, non-overlapping changes, that might be the "best" case. P1 sets A, B, and P2 sets C, D then the final result should be A, B, C and D. The "worst" case would be if the final values were A,B or C,D
>
>If the sets are overlapping then the result would be non-determinant P1 sets A, deletes B and P2 sets B and C, then B could be set or deleted. Changing the same property should be much less frequent than modifying non-overlapping sets.
I created #2967 to do the retry and in that PR I also added a rigorous concurrency test. The test makes lots of modifications in many threads and can detect if any single modification is lost. The test also has some thread modify the same properties.
Ideally when multiple threads are concurrently modifying properties, that should result in the same outcome as if all those modifications were done serially in some order. Barring bugs, conceptually I think the current accumulo code should be able to achieve this.
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r997093001
##########
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:
That makes sense, ill include this change.
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2965: Use TableOperations.modifyProperties in more places
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2965:
URL: https://github.com/apache/accumulo/pull/2965#discussion_r980471355
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsHelper.java:
##########
@@ -56,10 +57,12 @@ public void attachIterator(String tableName, IteratorSetting setting,
for (IteratorScope scope : scopes) {
String root = String.format("%s%s.%s", Property.TABLE_ITERATOR_PREFIX,
scope.name().toLowerCase(), setting.getName());
- for (Entry<String,String> prop : setting.getOptions().entrySet()) {
- this.setProperty(tableName, root + ".opt." + prop.getKey(), prop.getValue());
- }
- this.setProperty(tableName, root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ Map<String,String> propsToAdd = setting.getOptions().entrySet().stream()
+ .collect(Collectors.toMap(prop -> root + ".opt." + prop.getKey(), Entry::getValue));
+ propsToAdd.put(root, setting.getPriority() + "," + setting.getIteratorClass());
+
+ this.modifyProperties(tableName, props -> props.putAll(propsToAdd));
Review Comment:
> If no one else is working on it or opposed to it, I would like to try creating a PR for the new modifyProperties method to make it retry automatically instead of throwing the ConcurrentModificationException. If that was done, then this PR could stay as is and not have to handle that condition.
Sounds good to me :+1:
--
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