smiroslav commented on code in PR #2901: URL: https://github.com/apache/jackrabbit-oak/pull/2901#discussion_r3258716272
########## oak-segment-remote/src/test/java/org/apache/jackrabbit/oak/segment/remote/persistentcache/PersistentDiskCacheSizeAccountingTest.java: ########## @@ -0,0 +1,309 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package org.apache.jackrabbit.oak.segment.remote.persistentcache; + +import org.apache.commons.io.FileUtils; +import org.apache.jackrabbit.oak.segment.spi.persistence.persistentcache.AbstractPersistentCache; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.lang.reflect.Field; +import java.time.LocalDate; +import java.util.UUID; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +/** + * Regression tests for OAK-12212. + * + * <p>Before the OAK-12212 fix, the in-memory {@code cacheSize} counter + * maintained by {@link PersistentDiskCache} could drift well above the actual + * size of the cache directory: the {@code writesPending} guard in + * {@link PersistentDiskCache#writeSegment(long, long, org.apache.jackrabbit.oak.commons.Buffer)} + * only prevents <em>simultaneously running</em> write tasks for the same + * segment id, not <em>sequentially running</em> ones. Every {@code writeSegment} + * invocation that reached the body still added {@code fileSize} to + * {@code cacheSize}, yet on POSIX file systems {@code Files.move} with + * {@code ATOMIC_MOVE} maps to {@code rename(2)} and silently replaces an + * existing destination — so repeated writes of the same segment id produced + * a single file on disk but multiple increments of the in-memory counter. + * The cleanup path could only subtract the actual length of the (one) file + * it deleted, so the over-counted bytes were never repaid. + * + * <p>The fix is gated by {@link PersistentDiskCache#FT_OAK_12212_SKIP_MISSING_FILE_CHECK}. + * With the toggle in its default state (fix enabled), {@code writeSegment} + * short-circuits when the segment is already on disk; flipping the toggle + * restores the original behaviour for emergency rollback. + * + * <p>To make the tests deterministic, the cache's internal worker executor + * is replaced with a single-threaded one and explicitly drained after every + * {@code writeSegment} call using a marker task. + */ +public class PersistentDiskCacheSizeAccountingTest { + + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(new File("target")); + + private static final int SEGMENT_LEN = 256 * 1024; + + private File cacheFolder; + private DiskCacheIOMonitor ioMonitor; + private PersistentDiskCache persistentCache; + + /** + * Captures the running counter value reported via + * {@link DiskCacheIOMonitor#updateCacheSize(long, long)} every time the + * cache mutates its in-memory {@code cacheSize}. The latest value mirrors + * the current state of the (otherwise package-private) {@code AtomicLong}. + */ + private final AtomicLong lastReportedCacheSize = new AtomicLong(0); + + @Before + public void setUp() throws Exception { + cacheFolder = temporaryFolder.newFolder(); + ioMonitor = mock(DiskCacheIOMonitor.class); + doAnswer(inv -> { + lastReportedCacheSize.set(inv.getArgument(0, Long.class)); + return null; + }).when(ioMonitor).updateCacheSize(anyLong(), anyLong()); + // The OAK-12212 kill switch is a process-wide AtomicBoolean. Reset + // it before each test so previous tests cannot leak their toggle + // state into the next one. + PersistentDiskCache.FT_OAK_12212_SKIP_MISSING_FILE_CHECK.set(false); + } + + @After + public void tearDown() { + if (persistentCache != null) { + persistentCache.close(); + persistentCache = null; + } + PersistentDiskCache.FT_OAK_12212_SKIP_MISSING_FILE_CHECK.set(false); + } + + /** + * Writes the same segment id repeatedly with a maximum cache size big + * enough that the cleanup path never runs. + * + * <p>With the OAK-12212 fix enabled (default), only the first write + * actually touches disk and the counter stays at one segment size. + * Without the fix the cache directory still holds exactly one + * segment-sized file (POSIX rename silently replaces), but the + * in-memory counter is incremented {@code writes} times. + */ + @Test + public void cacheSizeCounterMustMatchDirectorySizeAfterRepeatedWritesOfSameSegment() + throws Exception { + // Cache big enough that the cleanup path never runs during the test. + persistentCache = new PersistentDiskCache(cacheFolder, /* maxCacheSizeMB */ 1024, ioMonitor); + replaceExecutorWithSingleThreaded(persistentCache); + + final byte[] segmentBytes = randomBytes(SEGMENT_LEN); + final UUID segmentId = UUID.randomUUID(); + final long msb = segmentId.getMostSignificantBits(); + final long lsb = segmentId.getLeastSignificantBits(); + final int writes = 4; // mirrors the ~4x drift seen in the heap dump + + for (int i = 0; i < writes; i++) { + persistentCache.writeSegment(msb, lsb, org.apache.jackrabbit.oak.commons.Buffer.wrap(segmentBytes)); Review Comment: here and in other places I wold rather use Buffer.wrapp and add org.apache.jackrabbit.oak.commons.Buffer to the import statements. -- 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]
