blambov commented on code in PR #2777:
URL: https://github.com/apache/cassandra/pull/2777#discussion_r1363517205
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -1150,6 +1151,13 @@ public enum DiskAccessMode
standard,
}
+ public enum CommitLogDiskAccessMode
+ {
+ standard,
+ mmap,
+ direct_io,
Review Comment:
`standard` and `mmap` are also I/O modes, I would drop the `_io` part to
`direct`.
##########
conf/cassandra.yaml:
##########
@@ -589,6 +589,10 @@ commitlog_segment_size: 32MiB
# parameters:
# -
+# Configure commitlog disk access mode preferred order:direct_io, mmap,
standard.
+# Default access mode is mmap. Direct I/O uses minimum page size to flush to
disk.
+commitlog_disk_access_mode : mmap
Review Comment:
The name of the option is good for me, but it is quite important to have a
more precise description.
> Some replies showed interest in Mmap as default to prevent any surprises.
This is what I expected. Take a look at CASSANDRA-18753 as a potential way
forward for making direct the default for new users.
##########
src/java/org/apache/cassandra/db/commitlog/CommitLogSegment.java:
##########
@@ -175,7 +177,10 @@ static long getNextId()
try
{
- channel = FileChannel.open(logFile.toPath(),
StandardOpenOption.WRITE, StandardOpenOption.READ, StandardOpenOption.CREATE);
+ if(commitLog.configuration.isDirectIOEnabled())
+ channel = FileChannel.open(logFile.toPath(),
StandardOpenOption.WRITE, StandardOpenOption.READ, StandardOpenOption.CREATE,
ExtendedOpenOption.DIRECT);
Review Comment:
Okay, I see that I did not understand how Java NIO's direct mode works and
we need to implement the paging/buffering ourselves, which is not as trivial.
One thing we can do for now is to only apply the direct mode if compression
or encryption are not enabled, falling back to standard access otherwise
(possibly with a message).
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -1149,6 +1151,13 @@ public enum DiskAccessMode
standard,
}
+ public enum CommitLogDiskAccessMode
+ {
+ standard,
Review Comment:
I see three problems with the `standard` option:
- there is no implementation of standard mode for uncompressed log (we
probably can use [this simple
one](https://github.com/datastax/cassandra/blob/ds-trunk/src/java/org/apache/cassandra/db/commitlog/UncompressedSegment.java)
if we need to have it);
- the current code requires the user to change from `direct` to `standard`
when enabling compression/encryption, which is not nice, and would be a deal
breaker for `direct` being the default;
- we understand `mmap` to silently translate to `standard` for
compressed/encrypted logs, which is legacy behaviour that we do not want to
change, but which is at odds with the above.
The simplest way to solve this is to explain in the option's comment that it
currently only applies to uncompressed/unencrypted logs and remove `standard`
from this enum. But if we are planning also to implement direct writing of
compressed/encrypted logs (valuable in its own right), it look like we should
change the alternative to `mmap_or_standard` or maybe something like `legacy`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
