You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by md...@apache.org on 2016/08/25 10:04:27 UTC
svn commit: r1757647 - in /jackrabbit/oak/trunk/oak-segment-tar/src:
main/java/org/apache/jackrabbit/oak/segment/
main/java/org/apache/jackrabbit/oak/segment/file/
test/java/org/apache/jackrabbit/oak/segment/
test/java/org/apache/jackrabbit/oak/segment...
Author: mduerig
Date: Thu Aug 25 10:04:27 2016
New Revision: 1757647
URL: http://svn.apache.org/viewvc?rev=1757647&view=rev
Log:
OAK-4700: Bulk segments might be cleaned up if they are not properly referenced
Make sure the SegmentBufferWriter keeps references to the segment ids of segments being built. Also include segment ids in the current tar writer referenced from a reader in the gc roots (bulk segments)
Added:
jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/TarWriterTest.java
Modified:
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java
jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/TarWriter.java
jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java?rev=1757647&r1=1757646&r2=1757647&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/SegmentBufferWriter.java Thu Aug 25 10:04:27 2016
@@ -40,7 +40,6 @@ import java.io.IOException;
import java.util.Collection;
import java.util.Map;
import java.util.Set;
-import java.util.UUID;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
@@ -108,7 +107,7 @@ public class SegmentBufferWriter impleme
*/
private final Map<RecordId, RecordType> roots = newLinkedHashMap();
- private final Set<UUID> referencedSegmentIds = newHashSet();
+ private final Set<SegmentId> referencedSegmentIds = newHashSet();
@Nonnull
private final SegmentStore store;
@@ -289,7 +288,7 @@ public class SegmentBufferWriter impleme
writeShort((short)((offset >> Segment.RECORD_ALIGN_BITS) & 0xffff));
if (!recordId.getSegmentId().equals(segment.getSegmentId())) {
- referencedSegmentIds.add(recordId.getSegmentId().asUUID());
+ referencedSegmentIds.add(recordId.getSegmentId());
}
statistics.recordIdCount++;
@@ -368,7 +367,7 @@ public class SegmentBufferWriter impleme
length = buffer.length;
}
- for (UUID id : referencedSegmentIds) {
+ for (SegmentId id : referencedSegmentIds) {
pos = BinaryUtils.writeLong(buffer, pos, id.getMostSignificantBits());
pos = BinaryUtils.writeLong(buffer, pos, id.getLeastSignificantBits());
}
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java?rev=1757647&r1=1757646&r2=1757647&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/FileStore.java Thu Aug 25 10:04:27 2016
@@ -825,11 +825,8 @@ public class FileStore implements Segmen
// to clear stale weak references in the SegmentTracker
System.gc();
- for (SegmentId id : tracker.getReferencedSegmentIds()) {
- if (!isDataSegmentId(id.getLeastSignificantBits())) {
- bulkRefs.add(id.asUUID());
- }
- }
+ collectBulkReferences(bulkRefs);
+
for (TarReader reader : readers) {
cleaned.put(reader, reader);
initialSize += reader.size();
@@ -913,6 +910,19 @@ public class FileStore implements Segmen
return toRemove;
}
+ private void collectBulkReferences(Set<UUID> bulkRefs) {
+ Set<UUID> refs = newHashSet();
+ for (SegmentId id : tracker.getReferencedSegmentIds()) {
+ refs.add(id.asUUID());
+ }
+ tarWriter.collectReferences(refs);
+ for (UUID ref : refs) {
+ if (!isDataSegmentId(ref.getLeastSignificantBits())) {
+ bulkRefs.add(ref);
+ }
+ }
+ }
+
/**
* Finds all external blob references that are currently accessible
* in this repository and adds them to the given collector. Useful
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/TarWriter.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/TarWriter.java?rev=1757647&r1=1757646&r2=1757647&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/TarWriter.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/TarWriter.java Thu Aug 25 10:04:27 2016
@@ -22,6 +22,8 @@ import static com.google.common.base.Cha
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkPositionIndexes;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.Lists.newArrayList;
+import static com.google.common.collect.Lists.reverse;
import static com.google.common.collect.Maps.newHashMap;
import static com.google.common.collect.Maps.newLinkedHashMap;
import static com.google.common.collect.Sets.newHashSet;
@@ -602,6 +604,27 @@ class TarWriter implements Closeable {
return header;
}
+ /**
+ * Add all segment ids that are reachable from {@code referencedIds} via
+ * this writer's segment graph and subsequently remove those segment ids
+ * from {@code referencedIds} that are in this {@code TarWriter}. The
+ * latter can't be cleaned up anyway because they are not be present in
+ * any of the readers.
+ *
+ * @param referencedIds
+ * @throws IOException
+ */
+ synchronized void collectReferences(Set<UUID> referencedIds) {
+ for (UUID uuid : reverse(newArrayList(index.keySet()))) {
+ if (referencedIds.remove(uuid)) {
+ Set<UUID> refs = graph.get(uuid);
+ if (refs != null) {
+ referencedIds.addAll(refs);
+ }
+ }
+ }
+ }
+
//------------------------------------------------------------< Object >--
@Override
Modified: jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java?rev=1757647&r1=1757646&r2=1757647&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java (original)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/CompactionAndCleanupIT.java Thu Aug 25 10:04:27 2016
@@ -22,6 +22,7 @@ package org.apache.jackrabbit.oak.segmen
import static com.google.common.collect.Lists.newArrayList;
import static com.google.common.util.concurrent.Uninterruptibles.sleepUninterruptibly;
import static java.lang.Integer.getInteger;
+import static java.lang.String.valueOf;
import static java.util.concurrent.Executors.newFixedThreadPool;
import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;
import static java.util.concurrent.TimeUnit.SECONDS;
@@ -1001,7 +1002,7 @@ public class CompactionAndCleanupIT {
ScheduledExecutorService scheduler = newSingleThreadScheduledExecutor();
StatisticsProvider statsProvider = new DefaultStatisticsProvider(scheduler);
final FileStoreGCMonitor fileStoreGCMonitor = new FileStoreGCMonitor(Clock.SIMPLE);
-
+
final FileStore fileStore = fileStoreBuilder(getFileStoreFolder())
.withGCOptions(defaultGCOptions().setRetainedGenerations(2))
.withGCMonitor(fileStoreGCMonitor)
@@ -1111,7 +1112,73 @@ public class CompactionAndCleanupIT {
fileStore.close();
}
}
-
+
+ /**
+ * Test asserting OAK-4700: Concurrent cleanup must not remove segments that are still reachable
+ */
+ @Test
+ public void concurrentCleanup() throws Exception {
+ File fileStoreFolder = getFileStoreFolder();
+
+ final FileStore fileStore = fileStoreBuilder(fileStoreFolder)
+ .withMaxFileSize(1)
+ .build();
+
+ final SegmentNodeStore nodeStore = SegmentNodeStoreBuilders.builder(fileStore).build();
+ ExecutorService executorService = newFixedThreadPool(50);
+ final AtomicInteger counter = new AtomicInteger();
+
+ try {
+ Callable<Void> concurrentWriteTask = new Callable<Void>() {
+ @Override
+ public Void call() throws Exception {
+ NodeBuilder builder = nodeStore.getRoot().builder();
+ builder.setProperty("blob-" + counter.getAndIncrement(), createBlob(nodeStore, 512 * 512));
+ nodeStore.merge(builder, EmptyHook.INSTANCE, CommitInfo.EMPTY);
+ fileStore.flush();
+ return null;
+ }
+ };
+
+ final Callable<Void> concurrentCleanTask = new Callable<Void>() {
+ @Override
+ public Void call() throws Exception {
+ // FIXME OAK-4685: Explicitly avoid concurrent cleanup calls until OAK-4685 is fixed
+ synchronized (nodeStore) {
+ fileStore.cleanup();
+ }
+ return null;
+ }
+ };
+
+ List<Future<?>> results = newArrayList();
+ for (int i = 0; i < 50; i++) {
+ if (i % 2 == 0) {
+ results.add(executorService.submit(concurrentWriteTask));
+ } else {
+ results.add(executorService.submit(concurrentCleanTask));
+ }
+ }
+
+ for (Future<?> result : results) {
+ assertNull(result.get());
+ }
+
+ for (String fileName : fileStoreFolder.list()) {
+ if (fileName.endsWith(".tar")) {
+ int pos = fileName.length() - "a.tar".length();
+ char generation = fileName.charAt(pos);
+ assertEquals("Expected nothing to be cleaned but generation '" + generation +
+ "' for file " + fileName + " indicates otherwise.",
+ "a", valueOf(generation));
+ }
+ }
+ } finally {
+ new ExecutorCloser(executorService).close();
+ fileStore.close();
+ }
+ }
+
private static void addContent(NodeBuilder builder) {
for (int k = 0; k < 10000; k++) {
builder.setProperty(UUID.randomUUID().toString(), UUID.randomUUID().toString());
Added: jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/TarWriterTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/TarWriterTest.java?rev=1757647&view=auto
==============================================================================
--- jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/TarWriterTest.java (added)
+++ jackrabbit/oak/trunk/oak-segment-tar/src/test/java/org/apache/jackrabbit/oak/segment/file/TarWriterTest.java Thu Aug 25 10:04:27 2016
@@ -0,0 +1,236 @@
+/*
+ * 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.file;
+
+import static com.google.common.collect.Maps.newHashMap;
+import static com.google.common.collect.Sets.newHashSet;
+import static java.nio.ByteBuffer.allocate;
+import static java.util.Collections.singleton;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.Map;
+import java.util.Set;
+import java.util.UUID;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.jackrabbit.oak.segment.RecordId;
+import org.apache.jackrabbit.oak.segment.Segment;
+import org.apache.jackrabbit.oak.segment.SegmentId;
+import org.apache.jackrabbit.oak.segment.SegmentWriter;
+import org.apache.jackrabbit.oak.segment.SegmentWriterBuilder;
+import org.apache.jackrabbit.oak.segment.file.TarWriterTest.SegmentGraphBuilder.Node;
+import org.apache.jackrabbit.oak.segment.memory.MemoryStore;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class TarWriterTest {
+
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder(new File("target"));
+
+ /**
+ * Regression test for OAK-2800
+ */
+ @Test
+ public void collectReferences() throws IOException {
+ SegmentGraphBuilder graphBuilder = new SegmentGraphBuilder();
+
+ // a -> b -> c
+ Node c = graphBuilder.createNode("c");
+ Node b = graphBuilder.createNode("b", c);
+ Node a = graphBuilder.createNode("a", b);
+
+ Node n = graphBuilder.createNode("n");
+
+ // y -> z
+ Node z = graphBuilder.createNode("z");
+ Node y = graphBuilder.createNode("y", z);
+
+ assertEquals(singleton(b), a.getReferences());
+ assertEquals(singleton(c), b.getReferences());
+ assertTrue(c.getReferences().isEmpty());
+ assertEquals(singleton(z), y.getReferences());
+ assertTrue(z.getReferences().isEmpty());
+
+ File tar = folder.newFile(getClass().getName() + ".tar");
+ try (TarWriter tarWriter = new TarWriter(tar)) {
+ y.write(tarWriter);
+ b.write(tarWriter);
+ a.write(tarWriter);
+ n.write(tarWriter);
+
+ Set<UUID> references = newHashSet();
+ references.add(a.getUUID());
+ tarWriter.collectReferences(references);
+ assertEquals(
+ c + " must be in references as " + a + " has an transitive reference to " + c + " through " + b + ", " +
+ a + " must not be in references as " + a + " is in the TarWriter, " +
+ "no other elements must be in references.",
+ singleton(c), toNodes(graphBuilder, references));
+
+ references.clear();
+ references.add(b.getUUID());
+ tarWriter.collectReferences(references);
+ assertEquals(
+ b + " must be in references as " + a + " has a direct reference to " + b + ", " +
+ a + " must not be in references as " + a + " is in the TarWriter, " +
+ "no other elements must be in references.",
+ singleton(c), toNodes(graphBuilder, references));
+
+ references.clear();
+ references.add(y.getUUID());
+ tarWriter.collectReferences(references);
+ assertEquals(
+ z + " must be in references as " + y + " has a direct reference to " + z + ", " +
+ y + " must not be in references as " + y + " is in the TarWriter, " +
+ "no other elements must be in references.",
+ singleton(z), toNodes(graphBuilder, references));
+
+ references.clear();
+ references.add(c.getUUID());
+ tarWriter.collectReferences(references);
+ assertEquals(
+ c + " must be in references as " + c + " is not in the TarWriter, " +
+ "no other elements must be in references.",
+ singleton(c), toNodes(graphBuilder, references));
+
+ references.clear();
+ references.add(z.getUUID());
+ tarWriter.collectReferences(references);
+ assertEquals(
+ z + " must be in references as " + z + " is not in the TarWriter " +
+ "no other elements must be in references.",
+ singleton(z), toNodes(graphBuilder, references));
+
+ references.clear();
+ references.add(n.getUUID());
+ tarWriter.collectReferences(references);
+ assertTrue(
+ "references must be empty as " + n + " has no references " +
+ "and " + n + " is in the TarWriter",
+ references.isEmpty());
+ }
+ }
+
+ private static Set<Node> toNodes(SegmentGraphBuilder graphBuilder, Set<UUID> uuids) {
+ Set<Node> nodes = newHashSet();
+ for (UUID uuid : uuids) {
+ nodes.add(graphBuilder.getNode(uuid));
+ }
+ return nodes;
+ }
+
+ public static class SegmentGraphBuilder {
+ private final Map<SegmentId, ByteBuffer> segments = newHashMap();
+ private final Map<UUID, Node> nodes = newHashMap();
+
+ private final MemoryStore store;
+ private final SegmentWriter writer;
+
+ private int nextNodeNo;
+
+ public SegmentGraphBuilder() throws IOException {
+ store = new MemoryStore() {
+ @Override
+ public void writeSegment(SegmentId id, byte[] data, int offset, int length) throws IOException {
+ super.writeSegment(id, data, offset, length);
+ ByteBuffer buffer = allocate(length);
+ buffer.put(data, offset, length);
+ buffer.rewind();
+ segments.put(id, buffer);
+ }
+ };
+ writer = SegmentWriterBuilder.segmentWriterBuilder("test-writer").build(store);
+ }
+
+ public class Node {
+ final String name;
+ final RecordId selfId;
+ final byte[] data;
+ final Segment segment;
+
+ Node(String name, RecordId selfId, ByteBuffer data) {
+ this.name = name;
+ this.selfId = selfId;
+ this.data = data.array();
+ segment = new Segment(store, store.getReader(), selfId.getSegmentId(), data);
+ }
+
+ public void write(TarWriter tarWriter) throws IOException {
+ long msb = getSegmentId().getMostSignificantBits();
+ long lsb = getSegmentId().getLeastSignificantBits();
+ tarWriter.writeEntry(msb, lsb, data, 0, data.length, 0);
+ for (Node node : getReferences()) {
+ tarWriter.addGraphEdge(selfId.asUUID(), node.getUUID());
+ }
+ }
+
+ public UUID getUUID() {
+ return selfId.asUUID();
+ }
+
+ private SegmentId getSegmentId() {
+ return selfId.getSegmentId();
+ }
+
+ public Set<Node> getReferences() {
+ Set<Node> references = newHashSet();
+ for (int k = 0; k < segment.getReferencedSegmentIdCount(); k++) {
+ references.add(nodes.get(segment.getReferencedSegmentId(k)));
+ }
+ references.remove(this);
+ return references;
+ }
+
+ @Override
+ public String toString() {
+ return name;
+ }
+
+ void addReference(SegmentWriter writer) throws IOException {
+ // Need to write a proper list as singleton lists are optimised
+ // to just returning the recordId of its single element
+ writer.writeList(ImmutableList.of(selfId, selfId));
+ }
+ }
+
+ public Node createNode(String name, Node... refs) throws IOException {
+ RecordId selfId = writer.writeString("id-" + nextNodeNo++);
+ for (Node ref : refs) {
+ ref.addReference(writer);
+ }
+ writer.flush();
+ SegmentId segmentId = selfId.getSegmentId();
+ Node node = new Node(name, selfId, segments.get(segmentId));
+ nodes.put(segmentId.asUUID(), node);
+ return node;
+ }
+
+ public Node getNode(UUID uuid) {
+ return nodes.get(uuid);
+ }
+ }
+
+}