Maxwell-Guo commented on code in PR #2777:
URL: https://github.com/apache/cassandra/pull/2777#discussion_r1360159877
##########
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:
I think we can rename this to commitlog_disk_access_mode_for_write or some
more concise words, or
may be better add some more detailed descriptions, to tell the users that
this configuration only work for commitlog write(sync) not for commitlog
read(replay)
I would prefer to add some description , so that when read commitlog at
replay stage, people will not need to modify this configuration .
And I think you should also give some description on mmap,
stand(buffer),direct_io , the difference between them.
##########
src/java/org/apache/cassandra/db/commitlog/CommitLog.java:
##########
@@ -129,6 +131,25 @@ private static CommitLog construct()
// register metrics
metrics.attach(executor, segmentManager);
+
+ String diskMode = " ";
+
+ switch (DatabaseDescriptor.getCommitLogDiskAccessMode())
+ {
+ case standard:
+ diskMode = "Standard (buffered)";
+ break;
+ case mmap:
+ diskMode = "MMap(memory mmaped)";
+ break;
+ case direct_io:
+ diskMode = "Direct (non-buffered)";
+ break;
+ default:
+ throw new IllegalArgumentException("Unknown commitlog disk
access mode type: " + DatabaseDescriptor.getCommitLogDiskAccessMode());
Review Comment:
` logger.info("Disk access mode for commitlog write is " +
DatabaseDescriptor.getCommitLogDiskAccessMode() );`
And if we add some detailed descriptions in the cassandra.yaml as what I
described upper , I think there may have no needs for this part here. Just
tell the user what disk mode we are using is enough.
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -390,6 +390,8 @@ public static class SSTableConfig
public ParameterizedClass commitlog_compression;
public FlushCompression flush_compression = FlushCompression.fast;
public int commitlog_max_compression_buffers_in_pool = 3;
+ public boolean direct_io_for_commitlog_enabled = false;
Review Comment:
what about remove this variable？And if we want to know wether we can use
direct ion ,just retun ommitLogDiskAccessMode.direct_io ==
commitlog_disk_access_mode
?
##########
src/java/org/apache/cassandra/db/commitlog/CommitLog.java:
##########
@@ -684,5 +713,32 @@ public EncryptionContext getEncryptionContext()
{
return encryptionContext;
}
+
+ /**
+ * Returns Direct-IO/non-buffer used for CommitLog IO.
+ * @return Direct-IO used for CommitLog IO
+ */
+ public boolean isDirectIOEnabled()
+ {
+ return diskAccessMode == Config.CommitLogDiskAccessMode.direct_io;
+ }
+
+ /**
+ * Returns MMAP used for CommitLog IO.
+ * @return MMAP used for CommitLog IO
+ */
+ public boolean isMMAPEnabled()
+ {
+ return diskAccessMode == Config.CommitLogDiskAccessMode.mmap;
+ }
+
+ /**
+ * Returns Standard or buffered I/O used for CommitLog IO.
+ * @return Standard or buffered I/O used for CommitLog IO
+ */
+ public boolean isBufferedIOEnabled()
+ {
+ return diskAccessMode == Config.CommitLogDiskAccessMode.standard;
+ }
Review Comment:
Do we need this method here ?
##########
src/java/org/apache/cassandra/config/Config.java:
##########
@@ -390,6 +390,8 @@ public static class SSTableConfig
public ParameterizedClass commitlog_compression;
public FlushCompression flush_compression = FlushCompression.fast;
public int commitlog_max_compression_buffers_in_pool = 3;
+ public boolean direct_io_for_commitlog_enabled = false;
Review Comment:
what about remove this variable？And if we want to know wether we can use
direct ion ,just retun ommitLogDiskAccessMode.direct_io ==
commitlog_disk_access_mode
?
##########
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:
as I have said before ，what about add a log for read ? if
commitlog_disk_access_mode is direct_io , and for read here, we can add a warn
log ,and tell the user, the direct io for read is not supported now, use the
default or other instand.
##########
src/java/org/apache/cassandra/db/commitlog/CommitLog.java:
##########
@@ -684,5 +713,32 @@ public EncryptionContext getEncryptionContext()
{
return encryptionContext;
}
+
+ /**
+ * Returns Direct-IO/non-buffer used for CommitLog IO.
+ * @return Direct-IO used for CommitLog IO
+ */
+ public boolean isDirectIOEnabled()
+ {
+ return diskAccessMode == Config.CommitLogDiskAccessMode.direct_io;
+ }
+
+ /**
+ * Returns MMAP used for CommitLog IO.
+ * @return MMAP used for CommitLog IO
+ */
+ public boolean isMMAPEnabled()
Review Comment:
Do we need this method here ?
##########
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:
I have a new suggestion here, for commitlog log replay , if the
configuration commitlog_disk_access_mode is direct_io , we can just logging
here : do not support directo io for commitlog replay, use default (stand)
here, so when we support direct io in the future , we will not need to
introduce a new configuration for read.
##########
test/conf/cassandra.yaml:
##########
@@ -8,6 +8,7 @@ memtable_allocation_type: offheap_objects
commitlog_sync: batch
commitlog_segment_size: 5MiB
commitlog_directory: build/test/cassandra/commitlog
+commitlog_disk_access_mode : mmap
Review Comment:
mmap ? Aren't we going to test direct_io ?
##########
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:
as I have said before ，what about add a log for read ? if
commitlog_disk_access_mode is direct_io , and for read here, we can add a warn
log ,and tell the user, the direct io for read is not supported now, use the
default or other instand.
--
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]
