keith-turner commented on code in PR #5988:
URL: https://github.com/apache/accumulo/pull/5988#discussion_r2557710321
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1280,7 +1280,9 @@ public enum Property {
"The durability used to write to the write-ahead log. Legal values are:"
+ " none, which skips the write-ahead log; log, which sends the data
to the"
+ " write-ahead log, but does nothing to make it durable; flush,
which pushes"
- + " data to the file system; and sync, which ensures the data is
written to disk.",
+ + " data to the file system; and sync, which ensures the data is
written to disk."
+ + " As of version 2.1.5 the value of sync will also cause RFile
blocks to be"
+ + " synced to disk when they are closed.",
Review Comment:
```suggestion
+ " synced to disk when they are closed. Other values of this
property do not change compaction file write behavior.",
```
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1280,7 +1280,9 @@ public enum Property {
"The durability used to write to the write-ahead log. Legal values are:"
+ " none, which skips the write-ahead log; log, which sends the data
to the"
+ " write-ahead log, but does nothing to make it durable; flush,
which pushes"
- + " data to the file system; and sync, which ensures the data is
written to disk.",
+ + " data to the file system; and sync, which ensures the data is
written to disk."
Review Comment:
Adding this behavior to the existing prop seems good because not sure one
would ever want them to differ. One potential problem is if this change in
behavior introduces a new problem in a bug fix release, then the the new file
behavior can not be turned of w/o changing walog behavior. Releasing a 2.1.6
would be one way to handle that though. For the long term this change seems
like the best one.
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/RFileOperations.java:
##########
@@ -96,15 +98,20 @@ protected FileSKVWriter openWriter(FileOptions options)
throws IOException {
AccumuloConfiguration acuconf = options.getTableConfiguration();
- long blockSize =
acuconf.getAsBytes(Property.TABLE_FILE_COMPRESSED_BLOCK_SIZE);
+ final long blockSize =
acuconf.getAsBytes(Property.TABLE_FILE_COMPRESSED_BLOCK_SIZE);
Preconditions.checkArgument((blockSize < Integer.MAX_VALUE && blockSize >
0),
"table.file.compress.blocksize must be greater than 0 and less than "
+ Integer.MAX_VALUE);
- long indexBlockSize =
acuconf.getAsBytes(Property.TABLE_FILE_COMPRESSED_BLOCK_SIZE_INDEX);
+ final long indexBlockSize =
acuconf.getAsBytes(Property.TABLE_FILE_COMPRESSED_BLOCK_SIZE_INDEX);
Preconditions.checkArgument((indexBlockSize < Integer.MAX_VALUE &&
indexBlockSize > 0),
"table.file.compress.blocksize.index must be greater than 0 and less
than "
+ Integer.MAX_VALUE);
+ // If the user has set the table durability to sync, then set the
SYNC_BLOCK flag
+ // on the RFiles so that each block is sync'd to disk when it's closed.
+ final boolean syncBlocks =
+ DurabilityImpl.fromString(acuconf.get(Property.TABLE_DURABILITY)) ==
Durability.SYNC;
Review Comment:
This could simplify later checks in the code, would only need to check if
`syncBlocks` is true.
```suggestion
DurabilityImpl.fromString(acuconf.get(Property.TABLE_DURABILITY)) ==
Durability.SYNC || options.dropCacheBehind;
```
--
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]
