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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
