aweisberg commented on code in PR #4606:
URL: https://github.com/apache/cassandra/pull/4606#discussion_r2813829036
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java:
##########
@@ -1417,44 +1419,60 @@ public StatsMetadata getSSTableMetadata()
return sstableMetadata;
}
+ public RandomAccessReader openDataReader()
+ {
+ return openDataReaderInternal(null, null, false);
+ }
+
public RandomAccessReader openDataReader(RateLimiter limiter)
{
assert limiter != null;
- return dfile.createReader(limiter);
+ return openDataReaderInternal(null, limiter, false);
}
- public RandomAccessReader openDataReader()
+ public RandomAccessReader openDataReader(DiskAccessMode diskAccessMode)
{
- return dfile.createReader();
+ return openDataReaderInternal(diskAccessMode, null, false);
}
public RandomAccessReader openDataReaderForScan()
{
- return openDataReaderForScan(dfile.diskAccessMode());
+ return openDataReaderInternal(null, null, true);
}
public RandomAccessReader openDataReaderForScan(DiskAccessMode
diskAccessMode)
{
- boolean isSameDiskAccessMode = diskAccessMode ==
dfile.diskAccessMode();
- boolean isDirectIONotSupported = diskAccessMode ==
DiskAccessMode.direct && !dfile.supportsDirectIO();
+ return openDataReaderInternal(diskAccessMode, null, true);
+ }
- if (isSameDiskAccessMode || isDirectIONotSupported)
- return dfile.createReaderForScan(OnReaderClose.RETAIN_FILE_OPEN);
+ private RandomAccessReader openDataReaderInternal(@Nullable DiskAccessMode
diskAccessMode,
Review Comment:
The logic in this method (and similar from the introduction of DIO for
scans) needs test coverage to make sure it can re-use when it is supposed to
re-use, and fall back to creating a new dfile, and then closes the dfile
correctly.
I haven't checked that these tests exist yet just pointing it out.
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java:
##########
@@ -1417,44 +1419,60 @@ public StatsMetadata getSSTableMetadata()
return sstableMetadata;
}
+ public RandomAccessReader openDataReader()
Review Comment:
Should there really be a no access mode arg versions here? Shouldn't
everyone know what access mode they want or explicitly supply they don't care?
Not sure if I am right here and the verbosity is worth it. The concern is
people call the no-arg version when they should have passed configuration, but
at some point it's not worth it.
##########
src/java/org/apache/cassandra/db/compaction/CursorCompactor.java:
##########
@@ -1553,6 +1547,33 @@ private static String mergeHistogramToString(long[]
histogram)
return sb.toString();
}
+ /**
+ * Closes scanner-opened readers before opening cursor-specific readers
with the configured disk access mode.
+ * In cursor compaction, scanners are only used for metadata; closing them
avoids holding redundant file
+ * descriptors and prevents conflicts when scan and non-scan readers for
the same file share thread-local
+ * buffer state on the same thread.
+ */
+ private static StatefulCursor[]
convertScannersToCursors(List<ISSTableScanner> scanners,
ImmutableSet<SSTableReader> sstables,
Review Comment:
This has some smell to it. Should the key in ThreadLocalReadAheadBuffer
include the disk access mode? Should these scanners have been opened with the
correct disk access mode in the first place or is it still an issue because we
then need to re-open with the Cursor based approach?
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java:
##########
@@ -1417,44 +1419,60 @@ public StatsMetadata getSSTableMetadata()
return sstableMetadata;
}
+ public RandomAccessReader openDataReader()
+ {
+ return openDataReaderInternal(null, null, false);
+ }
+
public RandomAccessReader openDataReader(RateLimiter limiter)
{
assert limiter != null;
- return dfile.createReader(limiter);
+ return openDataReaderInternal(null, limiter, false);
}
- public RandomAccessReader openDataReader()
+ public RandomAccessReader openDataReader(DiskAccessMode diskAccessMode)
{
- return dfile.createReader();
+ return openDataReaderInternal(diskAccessMode, null, false);
}
public RandomAccessReader openDataReaderForScan()
{
- return openDataReaderForScan(dfile.diskAccessMode());
+ return openDataReaderInternal(null, null, true);
}
public RandomAccessReader openDataReaderForScan(DiskAccessMode
diskAccessMode)
{
- boolean isSameDiskAccessMode = diskAccessMode ==
dfile.diskAccessMode();
- boolean isDirectIONotSupported = diskAccessMode ==
DiskAccessMode.direct && !dfile.supportsDirectIO();
+ return openDataReaderInternal(diskAccessMode, null, true);
+ }
- if (isSameDiskAccessMode || isDirectIONotSupported)
- return dfile.createReaderForScan(OnReaderClose.RETAIN_FILE_OPEN);
+ private RandomAccessReader openDataReaderInternal(@Nullable DiskAccessMode
diskAccessMode,
+ @Nullable RateLimiter
limiter,
+ boolean forScan)
+ {
+ if (canReuseDfile(diskAccessMode))
+ return dfile.createReader(limiter, forScan,
OnReaderClose.RETAIN_FILE_OPEN);
- FileHandle dataFile = dfile.toBuilder()
- .withDiskAccessMode(diskAccessMode)
- .complete();
+ FileHandle handle = dfile.toBuilder()
+ .withDiskAccessMode(diskAccessMode)
+ .complete();
try
{
- return dataFile.createReaderForScan(OnReaderClose.CLOSE_FILE);
+ return handle.createReader(limiter, forScan,
OnReaderClose.CLOSE_FILE);
}
catch (Throwable t)
{
- dataFile.close();
+ handle.close();
throw t;
}
}
+ private boolean canReuseDfile(@Nullable DiskAccessMode diskAccessMode)
+ {
+ return diskAccessMode == null
+ || diskAccessMode == dfile.diskAccessMode()
+ || (diskAccessMode == DiskAccessMode.direct &&
!dfile.supportsDirectIO());
Review Comment:
Do we at any point log that we weren't able to open the file with direct IO
as requested? Might be a use case for a rate limited logger.
##########
src/java/org/apache/cassandra/io/util/DirectThreadLocalReadAheadBuffer.java:
##########
@@ -46,4 +49,14 @@ protected void loadBlock(ByteBuffer blockBuffer, long
blockPosition, int sizeToR
if (channel.read(blockBuffer, blockPosition) < sizeToRead)
throw new CorruptSSTableException(null, channel.filePath());
}
+
+ @Override
+ protected void cleanBuffer(ByteBuffer buffer)
Review Comment:
Could this check be done in `MemoryUtil.clean` or should `MemoryUtil.clean`
have some kind of assertion added for cases where the buffer is a slice? I
imagine if we try to clean a slice we get some error on the native side, but
maybe it would be cleaner to error out in Java with a nice stack trace.
--
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]
