Copilot commented on code in PR #13487:
URL: https://github.com/apache/ignite/pull/13487#discussion_r3796185955
##########
modules/core/src/test/java/org/apache/ignite/internal/binary/streams/ThreadLocalAllocatorChunkShrinkTest.java:
##########
@@ -0,0 +1,113 @@
+package org.apache.ignite.internal.binary.streams;
+
+import java.lang.reflect.Field;
+import org.apache.ignite.IgniteCommonsSystemProperties;
+import org.junit.After;
Review Comment:
Unused import: IgniteCommonsSystemProperties is only referenced from Javadoc
and is not used by the compiler/checkstyle. This will typically fail static
checks; either remove the import or reference the type from code (not only
Javadoc).
##########
modules/core/src/test/java/org/apache/ignite/internal/binary/streams/ThreadLocalAllocatorChunkShrinkTest.java:
##########
@@ -0,0 +1,113 @@
+package org.apache.ignite.internal.binary.streams;
+
+import java.lang.reflect.Field;
+import org.apache.ignite.IgniteCommonsSystemProperties;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static
org.apache.ignite.IgniteCommonsSystemProperties.IGNITE_MARSHAL_BUFFERS_RECHECK;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * This test should demonstrate how ThreadLocalAllocator$Chunk shrinks if a
small message is written after a large one.
+ * <p>
+ * Shrink logic is executed only if enough time has passed to check size
again. For the ease of testing the
+ * corresponding system property is set to make the check run at every
invocation of
+ * {@link BinaryMemoryAllocatorChunk#release(byte[], int)}. The small message
size is set to less than half of the
+ * buffer size for the large message to make the chunk shrink.
+ * </p>
+ */
+public class ThreadLocalAllocatorChunkShrinkTest {
+ /** former value of System property {@link
IgniteCommonsSystemProperties#IGNITE_MARSHAL_BUFFERS_RECHECK}. */
+ private String oldIgniteMarshalBuffersRecheck;
+ /** output stream */
+ private BinaryHeapOutputStream outputStream;
+
+ /**
+ * Set recheck interval size to zero to always force check when closing
buffer.
+ * Clear Thread-Local chunk to equalize test startup condition.
+ */
+ @Before
+ public void init() {
+ oldIgniteMarshalBuffersRecheck =
System.getProperty(IGNITE_MARSHAL_BUFFERS_RECHECK);
+ System.setProperty(IGNITE_MARSHAL_BUFFERS_RECHECK, "0");
+ clearThreadLocalForBinaryMemoryAllocatorChunk();
+ }
+
+ /**
+ * First writes a large message to the stream then a small. If shrinking
does not happen stream and chunk array are
+ * the same which is why we use {@link BinaryStream#array()} for access.
It should fit the large message first.
+ * After writing the small message it should be reduced to half of its
former size.
+ */
+ @Test
+ public void testThreadLocalBufferShrinksAfterLargeMessage() {
+
+ int initSize = 128;
+ int largeMsgSize = 1024;
+
+ outputStream = new BinaryHeapOutputStream(initSize);
+ outputStream.writeByteArray(new byte[largeMsgSize]);
+ // Closing the stream invokes BinaryMemoryAllocatorChunk#release(...)
and thus the size check
+ closeOutputStream();
+
+ // new stream reuses thread-local chunk
+ outputStream = new BinaryHeapOutputStream(initSize);
+ // Stream array is assigned from chunk array only on creation of
stream. Query the new chunk size.
+ int largeBufSize = outputStream.array().length;
+
+ assertTrue(largeBufSize >= largeMsgSize);
+
+ outputStream.writeByte((byte)1);
+ closeOutputStream();
+
+ outputStream = new BinaryHeapOutputStream(initSize);
+
+ int expectedBufSize = largeBufSize >> 1;
+ int actualBufSize = outputStream.array().length;
+
+ assertEquals(expectedBufSize, actualBufSize);
+ closeOutputStream();
+ }
+
+ /**
+ * Restore system property and clear thread-local to not influence coming
up tests. Close stream if not already
+ * happened (e.g. in case of assertion error).
+ */
+ @After
+ public void cleanup() {
+
+ if (null == oldIgniteMarshalBuffersRecheck)
+ System.clearProperty(IGNITE_MARSHAL_BUFFERS_RECHECK);
+ else
+ System.setProperty(IGNITE_MARSHAL_BUFFERS_RECHECK,
oldIgniteMarshalBuffersRecheck);
+
+ closeOutputStream();
+ clearThreadLocalForBinaryMemoryAllocatorChunk();
+ }
+
+ /** Clears thread-local chunk. Since the field is private we use
reflection to gain access. */
+ private void clearThreadLocalForBinaryMemoryAllocatorChunk() {
+
+ try {
+ Field holdersField =
BinaryMemoryAllocator.THREAD_LOCAL.getClass().getDeclaredField("holders");
+ holdersField.setAccessible(true);
+ ThreadLocal<BinaryMemoryAllocatorChunk> holders =
(ThreadLocal<BinaryMemoryAllocatorChunk>)holdersField.get(
+ BinaryMemoryAllocator.THREAD_LOCAL);
+ holders.remove();
+ }
+ catch (NoSuchFieldException | IllegalAccessException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ /** Closes the output stream if not already happened */
+ private void closeOutputStream() {
+
+ if (outputStream != null) {
+ outputStream.close();
+ outputStream = null;
+ }
+ }
Review Comment:
This test relies on setting IGNITE_MARSHAL_BUFFERS_RECHECK to force the
shrink check on every close, but BinaryMemoryAllocator.CHECK_FREQ is computed
once at class initialization (Long.getLong(...)) and won’t change if
BinaryMemoryAllocator was already loaded by another test. That makes this test
order-dependent/flaky. Consider forcing the recheck deterministically in the
test (e.g., by setting the chunk’s lastCheckNanos far in the past before
closing).
--
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]
