aweisberg commented on code in PR #4606:
URL: https://github.com/apache/cassandra/pull/4606#discussion_r2897684215
##########
test/unit/org/apache/cassandra/db/compaction/CompactionsTest.java:
##########
@@ -260,8 +303,9 @@ public static void assertMaxTimestamp(ColumnFamilyStore
cfs, long maxTimestampEx
public void testUserDefinedCompaction() throws Exception
{
Keyspace keyspace = Keyspace.open(KEYSPACE1);
- final String cfname = "Standard3"; // use clean(no sstable) CF
+ final String cfname = "Standard3";
ColumnFamilyStore cfs = keyspace.getColumnFamilyStore(cfname);
+ cfs.clearUnsafe();
Review Comment:
Why did you need to add this?
##########
test/unit/org/apache/cassandra/io/util/DirectThreadLocalReadAheadBufferTest.java:
##########
@@ -61,4 +65,49 @@ private void testReads(InputData propertyInputs, int
bufferSize, int blockSize)
tlrab.close();
}
}
+
+ @Test
+ public void testDirectMemoryIsCleanedOnClose()
+ {
+ BufferPoolMXBean directPool = getDirectBufferPool();
+ int blockSize = FileUtils.getFileBlockSize(files[0]);
+ int bufferSize = 64 * 1024 * 1024; // 64MB - large enough to reliably
detect
+
+ try (ChannelProxy channel = new ChannelProxy(files[0],
ChannelProxy.IOMode.DIRECT))
+ {
+ DirectThreadLocalReadAheadBuffer tlrab =
+ new DirectThreadLocalReadAheadBuffer(channel, bufferSize,
blockSize);
+
+ // Force buffer allocation
+ tlrab.allocateBuffer();
+
+ long memoryUsedBefore = directPool.getMemoryUsed();
+
+ // Close should clean the direct memory
+ tlrab.close();
+
+ long memoryUsedAfter = directPool.getMemoryUsed();
+
+ // Memory should decrease by approximately buffer size (+
alignment overhead)
+ long expectedDecrease = bufferSize;
+ long actualDecrease = memoryUsedBefore - memoryUsedAfter;
+
+ Assert.assertTrue(
+ "Direct memory should decrease after close(). " +
+ "Before: " + memoryUsedBefore + ", After: " + memoryUsedAfter +
+ ", Expected decrease: ~" + expectedDecrease + ", Actual: " +
actualDecrease,
+ actualDecrease >= expectedDecrease * 0.9); // 10% tolerance
for alignment
+ }
+ }
+
+ private static BufferPoolMXBean getDirectBufferPool()
+ {
+ List<BufferPoolMXBean> pools =
ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class);
+ for (BufferPoolMXBean pool : pools)
+ if (pool.getName().equals("direct"))
+ return pool;
+
+ throw new IllegalStateException("Direct buffer pool not found");
+ }
+
Review Comment:
Extra line
##########
src/java/org/apache/cassandra/utils/memory/MemoryUtil.java:
##########
@@ -330,9 +331,13 @@ public static void clean(ByteBuffer buffer)
return;
DirectBuffer db = (DirectBuffer) buffer;
- if (db.attachment() != null)
- return; // duplicate or slice
-
- unsafe.invokeCleaner(buffer);
+ if (db.attachment() instanceof ByteBuffer)
Review Comment:
I think it would be fine to error out on other attachment types and put
together an allow list of attachments that aren't a problem. It seems like you
found a few issues with this check so it makes sense to be strict here and see
what else we don't know?
##########
src/java/org/apache/cassandra/io/util/DirectThreadLocalReadAheadBuffer.java:
##########
@@ -46,4 +49,12 @@ 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)
+ {
+ // Aligned buffers from BufferUtil.allocateDirectAligned are slices;
clean the backing buffer (attachment)
+ MemoryUtil.clean((ByteBuffer) ((DirectBuffer) buffer).attachment());
+ }
+
Review Comment:
Extra line
##########
test/unit/org/apache/cassandra/transport/WriteBytesTest.java:
##########
@@ -52,7 +55,11 @@ public void test()
Assertions.assertThat(buf.writerIndex()).isEqualTo(size);
for (int i = 0; i < size; i++)
Assertions.assertThat(buf.getByte(buf.readerIndex() +
i)).describedAs("byte mismatch at index %d", i).isEqualTo(bb.get(bb.position()
+ i));
- MemoryUtil.clean(bb);
+
+ if (bb.isDirect()) {
Review Comment:
Brace goes on next line
--
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]
