ACCUMULO-4629 Add table option to fail when deletes seen. #598
Conversation
| + " Valid actions are PROCESS and FAIL with PROCESS being the default. When set to " | ||
| + "PROCESS, deletes will supress data. When set to FAIL, any deletes seen will cause an" | ||
| + " exception. The purpose of FAIL is to support tables that never delete data and need " | ||
| + "fast seeks within the timestamp range of a column."), |
There was a problem hiding this comment.
For consistency, should make the values lowercase like TABLE_FILE_COMPRESSION_TYPE... "Possible values: zstd, gz, snappy, lzo, none". I don't think we have any uppercase property values...
There was a problem hiding this comment.
Could make it case-insensitive.
There was a problem hiding this comment.
It is case insensitive. It calls toUpperCase when parsing it.
| } | ||
| }; | ||
| default: | ||
| throw new IllegalArgumentException("Unknown behavior " + behavior); |
There was a problem hiding this comment.
This could be an AssertionError instead of IllegalArgumentException, since this class is responsible for defining what Behaviors are available, it's clearly programmer error on our part if we fail to handle one... rather than an API constraint imposed on the caller. It doesn't matter that much, but sometimes I think it's helpful to treat errors differently than exceptions.
| try { | ||
| scanner.forEach(e -> {}); | ||
| Assert.fail(); | ||
| } catch (RuntimeException e) {} |
There was a problem hiding this comment.
Useful to put a comment in the catch block to explain why it is expected. Also, a comment to the appropriate fail() param is helpful too, if a regression occurs.
I don't suppose we can get any more specific regarding the RTE type?
There was a problem hiding this comment.
I think the code throws a RTE.
| DeletingIterator delIter = new DeletingIterator(source, false); | ||
| byte[] defaultVisibility, AccumuloConfiguration conf) throws IOException { | ||
| SortedKeyValueIterator<Key,Value> delIter = DeletingIterator.wrap(source, false, | ||
| DeletingIterator.getBehavior(conf)); |
There was a problem hiding this comment.
Just a sanity check: is "conf" coming from the table configuration everywhere setupSystemScanIterators is called?
| } | ||
| } | ||
|
|
||
| public static Behavior getBehavior(AccumuloConfiguration conf) { |
There was a problem hiding this comment.
I would have expected this to appear on the ConfigurationTypeHelper class, with the other static methods that do the same kinds of thing. Putting it here makes sense in this case... but we might want to genericize this pattern of "enum" types with case-conversion and valueOf() lookup for generic enums there, and then this method's implementation becomes something like: return ConfigurationTypeHelper.getEnum(Property.TABLE_DELETE_BEHAVIOR, Behavior.class); or similar.
| @@ -114,7 +116,8 @@ | |||
| + " strategy, not the old one. (Most commonly, this will be because you have" | |||
| + " moved key material from one spot to another.) If you want to override" | |||
| + " the recorded key strategy with the one in the configuration file, set" | |||
| + " this property to true."), | |||
| + " this property to true. When setting this to fail, also consider" | |||
There was a problem hiding this comment.
This looks like it's in the wrong spot
This experimental option allows for tables that do not use deletes to have fast seeks within the timestamp range.
keith-turner
force-pushed
the
del-behavior
branch
from
e6defc3
to
59a889d
Compare
keith-turner
added
v2.0.0
enhancement
This experimental option allows for tables that do not use
deletes to have fast seeks within the timestamp range.