diff --git oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java index 6005e17..f06cfa0 100644 --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java @@ -833,7 +833,7 @@ public class FileStore implements SegmentStore, Closeable { long reclaimedSize = initialSize - afterCleanupSize; stats.reclaimed(reclaimedSize); - gcJournal.persist(finalSize); + gcJournal.persist(reclaimedSize, finalSize); gcListener.cleaned(reclaimedSize, finalSize); gcListener.info("TarMK GC #{}: cleanup completed in {} ({} ms). Post cleanup size is {} ({} bytes)" + " and space reclaimed {} ({} bytes).", diff --git oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStoreGCMonitor.java oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStoreGCMonitor.java index 923bb03..d36f9ab 100644 --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStoreGCMonitor.java +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStoreGCMonitor.java @@ -54,6 +54,9 @@ public class FileStoreGCMonitor extends AnnotatedStandardMBean private long lastCompaction; private long lastCleanup; + private long lastRepositorySize; + private long lastReclaimedSize; + private String lastError; private String status = "NA"; @@ -104,6 +107,9 @@ public class FileStoreGCMonitor extends AnnotatedStandardMBean @Override public void cleaned(long reclaimed, long current) { lastCleanup = clock.getTime(); + lastReclaimedSize = reclaimed; + lastRepositorySize = current; + gcCount.getCounter().addAndGet(1); repositorySize.getCounter().set(current); reclaimedSize.getCounter().addAndGet(reclaimed); @@ -120,7 +126,17 @@ public class FileStoreGCMonitor extends AnnotatedStandardMBean public String getLastCleanup() { return toString(lastCleanup); } - + + @Override + public long getLastRepositorySize() { + return lastRepositorySize; + } + + @Override + public long getLastReclaimedSize() { + return lastReclaimedSize; + } + private static String toString(long timestamp) { if (timestamp != 0) { return getDateTimeInstance().format(new Date(timestamp)); diff --git oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java index 6c3f5e4..e5a0024 100644 --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCJournal.java @@ -41,8 +41,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Persists the repository size following a cleanup operation in the - * {@link #GC_JOURNAL gc journal} file with the format: 'size, timestamp'. + * Persists the repository size and the reclaimed size following a cleanup operation in the + * {@link #GC_JOURNAL gc journal} file with the format: 'repoSize, reclaimedSize, timestamp'. */ public class GCJournal { @@ -59,8 +59,8 @@ public class GCJournal { this.directory = checkNotNull(directory); } - public synchronized void persist(long size) { - latest = new GCJournalEntry(size, System.currentTimeMillis()); + public synchronized void persist(long reclaimedSize, long repoSize) { + latest = new GCJournalEntry(reclaimedSize, repoSize, System.currentTimeMillis()); Path path = new File(directory, GC_JOURNAL).toPath(); try { try (BufferedWriter w = newBufferedWriter(path, UTF_8, WRITE, @@ -108,27 +108,30 @@ public class GCJournal { static class GCJournalEntry { - static GCJournalEntry EMPTY = new GCJournalEntry(-1, -1); + static GCJournalEntry EMPTY = new GCJournalEntry(-1, -1, -1); - private final long size; + private final long reclaimedSize; + private final long repoSize; private final long ts; - public GCJournalEntry(long size, long ts) { + public GCJournalEntry(long reclaimedSize, long repoSize, long ts) { + this.reclaimedSize = reclaimedSize; + this.repoSize = repoSize; this.ts = ts; - this.size = size; } @Override public String toString() { - return size + "," + ts; + return reclaimedSize + "," + repoSize + "," + ts; } static GCJournalEntry fromString(String in) { String[] items = in.split(","); - if (items.length == 2) { - long size = safeParse(items[0]); - long ts = safeParse(items[1]); - return new GCJournalEntry(size, ts); + if (items.length == 3) { + long reclaimedSize = safeParse(items[0]); + long repoSize = safeParse(items[1]); + long ts = safeParse(items[2]); + return new GCJournalEntry(reclaimedSize, repoSize, ts); } return GCJournalEntry.EMPTY; } @@ -142,8 +145,12 @@ public class GCJournal { return -1; } - public long getSize() { - return size; + public long getReclaimedSize() { + return reclaimedSize; + } + + public long getRepoSize() { + return repoSize; } public long getTs() { @@ -154,7 +161,8 @@ public class GCJournal { public int hashCode() { final int prime = 31; int result = 1; - result = prime * result + (int) (size ^ (size >>> 32)); + result = prime * result + (int) (reclaimedSize ^ (reclaimedSize >>> 32)); + result = prime * result + (int) (repoSize ^ (repoSize >>> 32)); result = prime * result + (int) (ts ^ (ts >>> 32)); return result; } @@ -168,7 +176,9 @@ public class GCJournal { if (getClass() != obj.getClass()) return false; GCJournalEntry other = (GCJournalEntry) obj; - if (size != other.size) + if (reclaimedSize != other.reclaimedSize) + return false; + if (repoSize != other.repoSize) return false; if (ts != other.ts) return false; diff --git oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCMonitorMBean.java oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCMonitorMBean.java index a53a6c1..f01bf13 100644 --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCMonitorMBean.java +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/GCMonitorMBean.java @@ -42,6 +42,16 @@ public interface GCMonitorMBean { */ @CheckForNull String getLastCleanup(); + + /** + * @return repository size after the last cleanup. + */ + long getLastRepositorySize(); + + /** + * @return reclaimed size during the last cleanup. + */ + long getLastReclaimedSize(); /** * @return last error or {@code null} if none. diff --git oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SizeDeltaGcEstimation.java oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SizeDeltaGcEstimation.java index ac05076..44c1a61 100644 --- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SizeDeltaGcEstimation.java +++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/SizeDeltaGcEstimation.java @@ -97,6 +97,6 @@ public class SizeDeltaGcEstimation implements GCEstimation { } private long getPreviousCleanupSize() { - return gcJournal.read().getSize(); + return gcJournal.read().getRepoSize(); } } diff --git oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java index 42ade59..abb583a 100644 --- oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java +++ oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java @@ -46,6 +46,7 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.FutureTask; import java.util.concurrent.ScheduledExecutorService; @@ -59,13 +60,17 @@ import org.apache.jackrabbit.oak.api.PropertyState; import org.apache.jackrabbit.oak.api.Type; import org.apache.jackrabbit.oak.segment.compaction.SegmentGCOptions; import org.apache.jackrabbit.oak.segment.file.FileStore; +import org.apache.jackrabbit.oak.segment.file.FileStoreGCMonitor; +import org.apache.jackrabbit.oak.segment.file.FileStoreStats; import org.apache.jackrabbit.oak.spi.commit.CommitInfo; import org.apache.jackrabbit.oak.spi.commit.EmptyHook; import org.apache.jackrabbit.oak.spi.state.ChildNodeEntry; import org.apache.jackrabbit.oak.spi.state.NodeBuilder; import org.apache.jackrabbit.oak.spi.state.NodeState; import org.apache.jackrabbit.oak.spi.state.NodeStore; +import org.apache.jackrabbit.oak.stats.Clock; import org.apache.jackrabbit.oak.stats.DefaultStatisticsProvider; +import org.apache.jackrabbit.oak.stats.StatisticsProvider; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -924,6 +929,56 @@ public class CompactionAndCleanupIT { fileStore.close(); } } + + /** + * Test asserting OAK-4106 : Concurrent writes during cleanup should not + * impact reclaimedSize computation. + */ + @Test + public void concurrentWritesDuringCleanup() throws Exception { + ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(); + StatisticsProvider statsProvider = new DefaultStatisticsProvider(executor); + FileStoreGCMonitor fileStoreGCMonitor = new FileStoreGCMonitor(Clock.SIMPLE); + + final FileStore fileStore = fileStoreBuilder(getFileStoreFolder()) + .withGCOptions(defaultGCOptions().setRetainedGenerations(2)) + .withGCMonitor(fileStoreGCMonitor) + .withStatisticsProvider(statsProvider) + .withMaxFileSize(1) + .build(); + + final SegmentNodeStore nodeStore = SegmentNodeStoreBuilders.builder(fileStore).build(); + + try { + Runnable concurrentWriteTask = new Runnable() { + public void run() { + try { + NodeBuilder builder = nodeStore.getRoot().builder(); + builder.setProperty("blob" + new Random().nextInt(), createBlob(nodeStore, 256 * 256)); + + nodeStore.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY); + fileStore.flush(); + } catch (CommitFailedException e) { + // ignore + } catch (IOException e) { + // ignore + } + }; + }; + + ExecutorService executorService = Executors.newFixedThreadPool(5); + for (int i = 0; i < 5; i++) { + executorService.execute(concurrentWriteTask); + } + + fileStore.cleanup(); + + assertTrue(fileStoreGCMonitor.getLastReclaimedSize() >= 0); + + } finally { + fileStore.close(); + } + } private static void addContent(NodeBuilder builder) { for (int k = 0; k < 10000; k++) { diff --git oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java index 37b1081..845a1f2 100644 --- oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java +++ oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/GcJournalTest.java @@ -44,20 +44,27 @@ public class GcJournalTest { File directory = segmentFolder.newFolder(); GCJournal gc = new GCJournal(directory); - gc.persist(100); + gc.persist(0, 100); GCJournalEntry e0 = gc.read(); - assertEquals(100, e0.getSize()); + assertEquals(0, e0.getReclaimedSize()); + assertEquals(100, e0.getRepoSize()); - gc.persist(250); + gc.persist(0, 250); GCJournalEntry e1 = gc.read(); - assertEquals(250, e1.getSize()); + assertEquals(0, e1.getReclaimedSize()); + assertEquals(250, e1.getRepoSize()); + + gc.persist(50, 200); + GCJournalEntry e2 = gc.read(); + assertEquals(50, e2.getReclaimedSize()); + assertEquals(200, e2.getRepoSize()); Collection all = gc.readAll(); - assertEquals(all.size(), 2); + assertEquals(all.size(), 3); File file = new File(directory, GCJournal.GC_JOURNAL); assertTrue(file.exists()); List allLines = Files.readAllLines(file.toPath(), UTF_8); - assertEquals(allLines.size(), 2); + assertEquals(allLines.size(), 3); } }