keith-turner commented on code in PR #5990:
URL: https://github.com/apache/accumulo/pull/5990#discussion_r2561154582
##########
core/src/main/java/org/apache/accumulo/core/client/admin/NewTableConfiguration.java:
##########
@@ -168,7 +168,7 @@ public Map<String,String> getProperties() {
Map<String,String> propertyMap = new HashMap<>();
if (limitVersion) {
-
propertyMap.putAll(IteratorConfigUtil.generateInitialTableProperties(limitVersion));
+ propertyMap.putAll(IteratorConfigUtil.getInitialTableProperties());
}
Review Comment:
This was probably not intended.
A good way to handle this would be to deprecate the
`withoutDefaultIterators()` method and leave its current confusing behavior and
add a new `withoutDefaults()` method to replace it that has much nicer
behavior. This avoids silently changing behavior behind an API and also it
makes the method names align better with actual behavior. Not sure about doing
this in a bug fix release, but it may be the best way to fix this bug w/o
making things worse.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java:
##########
@@ -150,29 +152,30 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
}
// Copy configuration options if flag was set
+ Map<String,String> srcTableConfig = null;
if (cl.hasOption(createTableOptCopyConfig.getOpt())) {
String srcTable = cl.getOptionValue(createTableOptCopyConfig.getOpt());
if (cl.hasOption(createTableOptExcludeParentProps.getLongOpt())) {
// copy properties, excludes parent properties in configuration
- Map<String,String> tableProps =
+ srcTableConfig =
shellState.getAccumuloClient().tableOperations().getTableProperties(srcTable);
- tableProps.entrySet().stream()
- .filter(entry -> Property.isValidTablePropertyKey(entry.getKey()))
- .forEach(entry -> initProperties.put(entry.getKey(),
entry.getValue()));
} else {
// copy configuration (include parent properties)
- final Map<String,String> configuration =
+ srcTableConfig =
shellState.getAccumuloClient().tableOperations().getConfiguration(srcTable);
- configuration.entrySet().stream()
- .filter(entry -> Property.isValidTablePropertyKey(entry.getKey()))
- .forEach(entry -> initProperties.put(entry.getKey(),
entry.getValue()));
}
+ srcTableConfig.entrySet().stream()
+ .filter(entry -> Property.isValidTablePropertyKey(entry.getKey()))
+ .forEach(entry -> initProperties.put(entry.getKey(),
entry.getValue()));
}
// if no defaults selected, remove, even if copied from configuration or
properties
if (cl.hasOption(createTableNoDefaultIters.getOpt())) {
- Set<String> initialProps =
IteratorConfigUtil.generateInitialTableProperties(true).keySet();
+ // handles if default props were copied over
+ Set<String> initialProps =
IteratorConfigUtil.getInitialTableProperties().keySet();
initialProps.forEach(initProperties::remove);
Review Comment:
This behavior is so confusing that I think we would be better off throwing
an exception here. I think the intent of this property is not to add default
iterators to a new table, but removing them makes this command non
deterministic. Would be good to be able to always predict what the create
table command will do for a given set of options. Seems to me throwing an
exception would be better than being unpredictable. Also throwing an exception
does not silently change the behavior of the command in the background which is
really hard to detect for any automation using this command.
```suggestion
if(!Collections.disjoint(initialProps, initProperties.keySet())){
throw //something
}
```
##########
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java:
##########
@@ -185,6 +188,18 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
shellState.getAccumuloClient().tableOperations().create(tableName,
ntc.setTimeType(timeType).setProperties(initProperties));
+ // any default properties not in the src table should also not be in the
dest table.
+ // Need to remove these after table creation.
+ if (cl.hasOption(createTableOptCopyConfig.getOpt())) {
+ var defaultProps =
IteratorConfigUtil.getInitialTableProperties().keySet();
+ Preconditions.checkState(srcTableConfig != null);
+ for (var defaultProp : defaultProps) {
+ if (srcTableConfig.get(defaultProp) == null) {
+
shellState.getAccumuloClient().tableOperations().removeProperty(tableName,
defaultProp);
+ }
+ }
+ }
Review Comment:
Could remove this
```suggestion
```
and before creating the table add the following..
```java
// TODO maybe do this for the createTableOptInitPropFile option also?
if (cl.hasOption(createTableOptCopyConfig.getOpt())) {
// copy table config exactly w/o setting any default table props
ntc = ntc.withoutDefaultIterators();
}
```
##########
test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java:
##########
@@ -454,4 +456,72 @@ public void getTimeTypeTest() throws
TableNotFoundException, AccumuloException,
"specified table does not exist");
}
+ @Test
+ public void testTablePropsEqual() throws Exception {
+ // Want to ensure users can somehow create a table via the Java API that
has exactly
+ // the same properties of another table (including changes to default
properties), similar to
+ // the copy config option of the create table Shell command. Cloning the
table is expected to
+ // be one way and modifying the properties after creation is expected to
be another way.
Review Comment:
Its super confusing, but there may be a way. I made another comment w/ a
suggestion on how to do this. Maybe we could improve this API in 4.0 to make
it less confusing.
##########
test/src/main/java/org/apache/accumulo/test/TableOperationsIT.java:
##########
@@ -454,4 +456,72 @@ public void getTimeTypeTest() throws
TableNotFoundException, AccumuloException,
"specified table does not exist");
}
+ @Test
+ public void testTablePropsEqual() throws Exception {
+ // Want to ensure users can somehow create a table via the Java API that
has exactly
+ // the same properties of another table (including changes to default
properties), similar to
+ // the copy config option of the create table Shell command. Cloning the
table is expected to
+ // be one way and modifying the properties after creation is expected to
be another way.
+
+ // default iterator property which is automatically set on creation of all
tables; want to
+ // ensure removal and changing this type of prop is retained on copying to
another table
+ final String defaultScanIterProp = Property.TABLE_ITERATOR_PREFIX
+ + IteratorUtil.IteratorScope.scan.name().toLowerCase() + ".vers";
+ final String defaultScanIterPropMaxVers = Property.TABLE_ITERATOR_PREFIX
+ + IteratorUtil.IteratorScope.scan.name().toLowerCase() +
".vers.opt.maxVersions";
+ final String defaultScanIterPropMaxVersDefault = "1";
+ final String defaultScanIterPropMaxVersNew = "999";
+ final String customTableProp =
Property.TABLE_ARBITRARY_PROP_PREFIX.getKey() + "foo";
+ final String customTablePropVal = "bar";
+ final String[] names = getUniqueNames(3);
+ final String table1 = names[0];
+ final String table2 = names[1];
+ final String table3 = names[2];
+
+ try (AccumuloClient client =
Accumulo.newClient().from(getClientProps()).build()) {
+ client.tableOperations().create(table1,
+ new NewTableConfiguration().setProperties(Map.of(customTableProp,
customTablePropVal)));
+
+ Wait.waitFor(() -> {
+ var table1Props = client.tableOperations().getTableProperties(table1);
+ return table1Props.get(customTableProp).equals(customTablePropVal)
+ && table1Props.get(defaultScanIterProp) != null && table1Props
+
.get(defaultScanIterPropMaxVers).equals(defaultScanIterPropMaxVersDefault);
+ });
+
+ // testing both modifying and deleting a default prop
+ client.tableOperations().modifyProperties(table1, props -> {
+ props.replace(defaultScanIterPropMaxVers,
defaultScanIterPropMaxVersNew);
+ props.remove(defaultScanIterProp);
+ });
+
+ Wait.waitFor(() -> {
+ var table1Props = client.tableOperations().getTableProperties(table1);
+ return table1Props.get(customTableProp).equals(customTablePropVal)
+ && table1Props.get(defaultScanIterProp) == null
+ &&
table1Props.get(defaultScanIterPropMaxVers).equals(defaultScanIterPropMaxVersNew);
+ });
+
+ client.tableOperations().create(table2);
+ client.tableOperations().modifyProperties(table2, props -> {
+ try {
+ props.clear();
+ props.putAll(client.tableOperations().getTableProperties(table1));
+ } catch (AccumuloException | TableNotFoundException e) {
+ throw new RuntimeException(e);
+ }
+ });
Review Comment:
```suggestion
// create table using exact properties from table1 w/o adding the
default iterators.
client.tableOperations().create(table2, new
NewTableConfiguration().withoutDefaultIterators().setProperties(client.tableOperations().getTableProperties(table1)));
```
##########
test/src/main/java/org/apache/accumulo/test/NewTableConfigurationIT.java:
##########
@@ -517,6 +517,38 @@ public void testNtcChaining() throws AccumuloException,
AccumuloSecurityExceptio
}
}
+ @Test
+ public void testIteratorConflictsWithDefault() throws Exception {
Review Comment:
It would be good to make the code in NewTableConfiguration that checks if
inputs are disjoint detect this case and fail. That may make code that was
previously working fail, but there is a good chance that code was buggy.
--
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]
