This is an automated email from the ASF dual-hosted git repository. miroslav pushed a commit to branch issue/OAK-10636 in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
commit 8e892b0d2008c33b837da6f1b17a42cb3a99f9c8 Author: Miroslav Smiljanic <[email protected]> AuthorDate: Tue Feb 6 11:51:33 2024 +0100 OAK-10636 check if TarFiles is initialised --- .../jackrabbit/oak/segment/file/FileStore.java | 2 +- .../oak/segment/file/ReadOnlyFileStore.java | 2 +- .../jackrabbit/oak/segment/file/tar/TarFiles.java | 39 +++++++++++---- .../oak/segment/file/tar/TarFilesTest.java | 55 ++++++++++++++++++++-- 4 files changed, 81 insertions(+), 17 deletions(-) diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java index c62304189f..6381e1129c 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java @@ -170,7 +170,7 @@ public class FileStore extends AbstractFileStore { .withPersistence(builder.getPersistence()) .withReaderCountStats(readerCountStats) .withSegmentCountStats(segmentCountStats) - .withInitialisedReaders(false); + .withInitialisedReadersAndWriters(false); this.tarFiles = tarFilesBuilder.build(); this.tarFiles.init(); diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/ReadOnlyFileStore.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/ReadOnlyFileStore.java index b90181637c..97b2dc7ef7 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/ReadOnlyFileStore.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/ReadOnlyFileStore.java @@ -77,7 +77,7 @@ public class ReadOnlyFileStore extends AbstractFileStore { .withMemoryMapping(memoryMapping) .withReadOnly() .withPersistence(builder.getPersistence()) - .withInitialisedReaders(false) + .withInitialisedReadersAndWriters(false) .build(); tarFiles.init(); writer = defaultSegmentWriterBuilder("read-only").withoutCache().build(this); diff --git a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarFiles.java b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarFiles.java index 31180e4653..fbd48e15fc 100644 --- a/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarFiles.java +++ b/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarFiles.java @@ -49,6 +49,7 @@ import java.util.regex.Pattern; import org.apache.jackrabbit.guava.common.base.Predicate; import org.apache.jackrabbit.guava.common.collect.Iterables; +import org.apache.jackrabbit.oak.api.IllegalRepositoryStateException; import org.apache.jackrabbit.oak.commons.Buffer; import org.apache.jackrabbit.oak.segment.file.FileReaper; import org.apache.jackrabbit.oak.segment.spi.monitor.FileStoreMonitor; @@ -136,7 +137,7 @@ public class TarFiles implements Closeable { private CounterStats segmentCountStats = NoopStats.INSTANCE; - private boolean initialisedReaders = true; + private boolean initialiseReadersAndWriters = true; private Builder() { // Prevent external instantiation. @@ -203,8 +204,8 @@ public class TarFiles implements Closeable { return this; } - public Builder withInitialisedReaders(boolean initialisedReaders) { - this.initialisedReaders = initialisedReaders; + public Builder withInitialisedReadersAndWriters(boolean initialiseReadersAndWriters) { + this.initialiseReadersAndWriters = initialiseReadersAndWriters; return this; } @@ -380,6 +381,11 @@ public class TarFiles implements Closeable { private final TarRecovery tarRecovery; + /** + * If {@code true}, the readers and writers are initialised. + */ + private boolean initialised; + private static int getSegmentCount(TarReader reader) { return reader.getEntries().length; } @@ -392,7 +398,7 @@ public class TarFiles implements Closeable { readOnly = builder.readOnly; tarRecovery = builder.tarRecovery; - if (builder.initialisedReaders) { + if (builder.initialiseReadersAndWriters) { init(); } } @@ -418,14 +424,21 @@ public class TarFiles implements Closeable { readers = new Node(r, readers); readerCount.inc(); } - if (readOnly) { - return; + if (!readOnly) { + int writeNumber = 0; + if (indices.length > 0) { + writeNumber = indices[indices.length - 1] + 1; + } + writer = new TarWriter(archiveManager, writeNumber, segmentCount); } - int writeNumber = 0; - if (indices.length > 0) { - writeNumber = indices[indices.length - 1] + 1; + + initialised = true; + } + + private void checkInitialised() { + if (!initialised) { + throw new IllegalRepositoryStateException("TarFiles not initialised"); } - writer = new TarWriter(archiveManager, writeNumber, segmentCount); } @Override @@ -549,6 +562,7 @@ public class TarFiles implements Closeable { } public void flush() throws IOException { + checkInitialised(); lock.readLock().lock(); try { writer.flush(); @@ -611,6 +625,7 @@ public class TarFiles implements Closeable { } public void writeSegment(UUID id, byte[] buffer, int offset, int length, GCGeneration generation, Set<UUID> references, Set<String> binaryReferences) throws IOException { + checkInitialised(); lock.writeLock().lock(); try { long size = writer.writeEntry( @@ -662,6 +677,7 @@ public class TarFiles implements Closeable { } void newWriter() throws IOException { + checkInitialised(); lock.writeLock().lock(); try { internalNewWriter(); @@ -671,6 +687,7 @@ public class TarFiles implements Closeable { } public CleanupResult cleanup(CleanupContext context) throws IOException { + checkInitialised(); CleanupResult result = new CleanupResult(); result.removableFiles = new ArrayList<>(); result.reclaimedSegmentIds = new HashSet<>(); @@ -822,6 +839,7 @@ public class TarFiles implements Closeable { } public void collectBlobReferences(Consumer<String> collector, Predicate<GCGeneration> reclaim) throws IOException { + checkInitialised(); Node head; lock.writeLock().lock(); try { @@ -908,6 +926,7 @@ public class TarFiles implements Closeable { } public FileReaper createFileReaper() { + checkInitialised(); return new FileReaper(archiveManager); } } diff --git a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/tar/TarFilesTest.java b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/tar/TarFilesTest.java index 39deea6ea2..37693d6ede 100644 --- a/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/tar/TarFilesTest.java +++ b/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/tar/TarFilesTest.java @@ -24,11 +24,7 @@ import static java.util.Collections.singleton; import static java.util.Collections.singletonList; import static java.util.UUID.randomUUID; import static org.apache.jackrabbit.oak.segment.file.tar.GCGeneration.newGCGeneration; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import java.io.File; import java.io.IOException; @@ -40,6 +36,7 @@ import java.util.Random; import java.util.Set; import java.util.UUID; +import org.apache.jackrabbit.oak.api.IllegalRepositoryStateException; import org.apache.jackrabbit.oak.commons.Buffer; import org.apache.jackrabbit.oak.segment.file.tar.TarFiles.CleanupResult; import org.apache.jackrabbit.oak.segment.spi.monitor.FileStoreMonitorAdapter; @@ -397,4 +394,52 @@ public class TarFilesTest { assertTrue(result.getReclaimedSegmentIds().isEmpty()); assertEquals(0, result.getReclaimedSize()); } + + @Test + public void testUninitialisedTarFiles() throws IOException { + tarFiles = TarFiles.builder() + .withDirectory(folder.getRoot()) + .withTarRecovery((id, data, recovery) -> { + // Intentionally left blank + }) + .withIOMonitor(new IOMonitorAdapter()) + .withFileStoreMonitor(new FileStoreMonitorAdapter()) + .withMaxFileSize(MAX_FILE_SIZE) + .withRemoteStoreMonitor(new RemoteStoreMonitorAdapter()) + .withInitialisedReadersAndWriters(false) + .build(); + + assertEquals(0, tarFiles.readerCount()); + assertEquals(0, tarFiles.segmentCount()); + assertEquals(0, tarFiles.size()); + assertFalse(tarFiles.containsSegment(0l, 0l)); + assertNull(tarFiles.readSegment(0l, 0l)); + + tarFiles.toString(); + assertThrows(IllegalRepositoryStateException.class, () -> tarFiles.flush()); + assertThrows(IllegalRepositoryStateException.class, () -> writeSegment(randomUUID())); + assertThrows(IllegalRepositoryStateException.class, () -> tarFiles.cleanup(new CleanupContext() { + @Override + public Collection<UUID> initialReferences() { + return emptySet(); + } + + @Override + public boolean shouldReclaim(UUID id, GCGeneration generation, boolean referenced) { + return false; + } + + @Override + public boolean shouldFollow(UUID from, UUID to) { + return false; + } + })); + + assertThrows(IllegalRepositoryStateException.class, () -> tarFiles.collectBlobReferences(r -> {}, g -> false)); + assertFalse(tarFiles.getSegmentIds().iterator().hasNext()); + assertTrue(tarFiles.getIndices().isEmpty()); + assertTrue(tarFiles.getGraph("").isEmpty()); + + assertThrows(IllegalRepositoryStateException.class, () -> tarFiles.createFileReaper()); + } }
