From 753fbaf11a5567915d9eaaa9e07ee1a93674c44e Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 21 Jan 2020 19:06:30 +0100 Subject: [PATCH] Simplify ReftableCompactor The ReftableCompactor supported a byteLimit, but this is currently unused. The FileReftableStack has a more sophisticated strategy that amortizes compaction costs. Rename min/maxUpdateIndex to reflogExpire{Min,Max}UpdateIndex to reflect their purpose more accurately. Since reflogs are generally pruned chronologically (oldest entries are expired first), one can only prune entries on full compaction, so they should not be set by default. Rephrase the function Reader#minUpdateIndex and maxUpdateIndex. These vars are documented to affect log entries, but semantically, they are about ref entries. Since ref entries have their timestamps delta-compressed, it is important for the min/maxUpdateIndex values to be coherent between different tables. The logical timestamps for log entries do not have to be coherent in different tables, as the timestamps of a log entry is part of the key. For example, a table written at update index 20 may contain a tombstone log entry at timestamp 1. Therefore, we set ReftableWriter's min/maxUpdateIndex from the merged tables we are compacting, rather than from the compaction settings (which should only control reflog expiry.) The previous behavior could drop log entries erroneously, especially in the presence of tombstone log entries. Unfortunately, testing this properly requires both an API for adding log tombstones, and a more refined API for controlling automatic compaction. Hence, no test. Change-Id: I2f4eb7866f607fddd0629809e8e61f0b9097717f Signed-off-by: Han-Wen Nienhuys --- .../reftable/ReftableCompactorTest.java | 6 +- .../storage/file/FileReftableStack.java | 4 - .../storage/reftable/MergedReftable.java | 9 ++ .../internal/storage/reftable/Reftable.java | 19 ++-- .../storage/reftable/ReftableCompactor.java | 89 ++++--------------- .../storage/reftable/ReftableReader.java | 10 +-- 6 files changed, 47 insertions(+), 90 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableCompactorTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableCompactorTest.java index 9a7f24095..0947dd210 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableCompactorTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableCompactorTest.java @@ -52,7 +52,9 @@ import static org.junit.Assert.assertTrue; import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import org.eclipse.jgit.internal.storage.io.BlockSource; import org.eclipse.jgit.internal.storage.reftable.ReftableWriter.Stats; @@ -96,7 +98,9 @@ public class ReftableCompactorTest { ReftableCompactor compactor; try (ByteArrayOutputStream outBuf = new ByteArrayOutputStream()) { compactor = new ReftableCompactor(outBuf); - compactor.tryAddFirst(read(inTab)); + List readers = new ArrayList<>(); + readers.add(read(inTab)); + compactor.addAll(readers); compactor.compact(); outTab = outBuf.toByteArray(); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java index cc52bfb71..7abd1fb28 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java @@ -477,10 +477,6 @@ public class FileReftableStack implements AutoCloseable { try (FileOutputStream fos = new FileOutputStream(tmpTable)) { ReftableCompactor c = new ReftableCompactor(fos) .setConfig(reftableConfig()) - .setMinUpdateIndex( - stack.get(first).reftableReader.minUpdateIndex()) - .setMaxUpdateIndex( - stack.get(last).reftableReader.maxUpdateIndex()) .setIncludeDeletes(first > 0); List compactMe = new ArrayList<>(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/MergedReftable.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/MergedReftable.java index 0144453cd..d272d09f0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/MergedReftable.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/MergedReftable.java @@ -98,6 +98,15 @@ public class MergedReftable extends Reftable { : 0; } + /** + * {@inheritDoc} + */ + @Override + public long minUpdateIndex() throws IOException { + return tables.length > 0 ? tables[0].minUpdateIndex() + : 0; + } + /** {@inheritDoc} */ @Override public boolean hasObjectMap() throws IOException { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/Reftable.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/Reftable.java index 8a8a14375..a7e8252b4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/Reftable.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/Reftable.java @@ -98,19 +98,28 @@ public abstract class Reftable { includeDeletes = deletes; } - /** - * Get the maximum update index for log entries that appear in this + * Get the maximum update index for ref entries that appear in this * reftable. * - * @return the maximum update index for log entries that appear in this - * reftable. This should be 1 higher than the prior reftable's - * {@code maxUpdateIndex} if this table is used in a stack. + * @return the maximum update index for ref entries that appear in this + * reftable. * @throws java.io.IOException * file cannot be read. */ public abstract long maxUpdateIndex() throws IOException; + /** + * Get the minimum update index for ref entries that appear in this + * reftable. + * + * @return the minimum update index for ref entries that appear in this + * reftable. + * @throws java.io.IOException + * file cannot be read. + */ + public abstract long minUpdateIndex() throws IOException; + /** * Seek to the first reference, to iterate in order. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableCompactor.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableCompactor.java index c73c245be..a586d42de 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableCompactor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableCompactor.java @@ -61,22 +61,20 @@ import org.eclipse.jgit.lib.ReflogEntry; * to shadow any lower reftable that may have the reference present. *

* By default all log entries within the range defined by - * {@link #setMinUpdateIndex(long)} and {@link #setMaxUpdateIndex(long)} are + * {@link #setReflogExpireMinUpdateIndex(long)} and {@link #setReflogExpireMaxUpdateIndex(long)} are * copied, even if no references in the output file match the log records. * Callers may truncate the log to a more recent time horizon with - * {@link #setOldestReflogTimeMillis(long)}, or disable the log altogether with + * {@link #setReflogExpireOldestReflogTimeMillis(long)}, or disable the log altogether with * {@code setOldestReflogTimeMillis(Long.MAX_VALUE)}. */ public class ReftableCompactor { private final ReftableWriter writer; private final ArrayDeque tables = new ArrayDeque<>(); - private long compactBytesLimit; - private long bytesToCompact; private boolean includeDeletes; - private long minUpdateIndex = -1; - private long maxUpdateIndex; - private long oldestReflogTimeMillis; + private long reflogExpireMinUpdateIndex = 0; + private long reflogExpireMaxUpdateIndex = Long.MAX_VALUE; + private long reflogExpireOldestReflogTimeMillis; private Stats stats; /** @@ -102,18 +100,6 @@ public class ReftableCompactor { return this; } - /** - * Set limit on number of bytes from source tables to compact. - * - * @param bytes - * limit on number of bytes from source tables to compact. - * @return {@code this} - */ - public ReftableCompactor setCompactBytesLimit(long bytes) { - compactBytesLimit = bytes; - return this; - } - /** * Whether to include deletions in the output, which may be necessary for * partial compaction. @@ -139,8 +125,8 @@ public class ReftableCompactor { * in a stack. * @return {@code this} */ - public ReftableCompactor setMinUpdateIndex(long min) { - minUpdateIndex = min; + public ReftableCompactor setReflogExpireMinUpdateIndex(long min) { + reflogExpireMinUpdateIndex = min; return this; } @@ -155,8 +141,8 @@ public class ReftableCompactor { * used in a stack. * @return {@code this} */ - public ReftableCompactor setMaxUpdateIndex(long max) { - maxUpdateIndex = max; + public ReftableCompactor setReflogExpireMaxUpdateIndex(long max) { + reflogExpireMaxUpdateIndex = max; return this; } @@ -170,16 +156,13 @@ public class ReftableCompactor { * Specified in Java standard milliseconds since the epoch. * @return {@code this} */ - public ReftableCompactor setOldestReflogTimeMillis(long timeMillis) { - oldestReflogTimeMillis = timeMillis; + public ReftableCompactor setReflogExpireOldestReflogTimeMillis(long timeMillis) { + reflogExpireOldestReflogTimeMillis = timeMillis; return this; } /** * Add all of the tables, in the specified order. - *

- * Unconditionally adds all tables, ignoring the - * {@link #setCompactBytesLimit(long)}. * * @param readers * tables to compact. Tables should be ordered oldest first/most @@ -191,44 +174,7 @@ public class ReftableCompactor { public void addAll(List readers) throws IOException { for (ReftableReader r : readers) { tables.add(r); - adjustUpdateIndexes(r); - } - } - - /** - * Try to add this reader at the bottom of the stack. - *

- * A reader may be rejected by returning {@code false} if the compactor is - * already rewriting its {@link #setCompactBytesLimit(long)}. When this - * happens the caller should stop trying to add tables, and execute the - * compaction. - * - * @param reader - * the reader to insert at the bottom of the stack. Caller is - * responsible for closing the reader. - * @return {@code true} if the compactor accepted this table; {@code false} - * if the compactor has reached its limit. - * @throws java.io.IOException - * if size of {@code reader}, or its update indexes cannot be read. - */ - public boolean tryAddFirst(ReftableReader reader) throws IOException { - long sz = reader.size(); - if (compactBytesLimit > 0 && bytesToCompact + sz > compactBytesLimit) { - return false; } - bytesToCompact += sz; - adjustUpdateIndexes(reader); - tables.addFirst(reader); - return true; - } - - private void adjustUpdateIndexes(ReftableReader reader) throws IOException { - if (minUpdateIndex == -1) { - minUpdateIndex = reader.minUpdateIndex(); - } else { - minUpdateIndex = Math.min(minUpdateIndex, reader.minUpdateIndex()); - } - maxUpdateIndex = Math.max(maxUpdateIndex, reader.maxUpdateIndex()); } /** @@ -241,8 +187,9 @@ public class ReftableCompactor { MergedReftable mr = new MergedReftable(new ArrayList<>(tables)); mr.setIncludeDeletes(includeDeletes); - writer.setMinUpdateIndex(Math.max(minUpdateIndex, 0)); - writer.setMaxUpdateIndex(maxUpdateIndex); + writer.setMaxUpdateIndex(mr.maxUpdateIndex()); + writer.setMinUpdateIndex(mr.minUpdateIndex()); + writer.begin(); mergeRefs(mr); mergeLogs(mr); @@ -268,16 +215,14 @@ public class ReftableCompactor { } private void mergeLogs(MergedReftable mr) throws IOException { - if (oldestReflogTimeMillis == Long.MAX_VALUE) { + if (reflogExpireOldestReflogTimeMillis == Long.MAX_VALUE) { return; } try (LogCursor lc = mr.allLogs()) { while (lc.next()) { long updateIndex = lc.getUpdateIndex(); - if (updateIndex < minUpdateIndex - || updateIndex > maxUpdateIndex) { - // Cannot merge log records outside the header's range. + if (updateIndex > reflogExpireMaxUpdateIndex || updateIndex < reflogExpireMinUpdateIndex) { continue; } @@ -291,7 +236,7 @@ public class ReftableCompactor { } PersonIdent who = log.getWho(); - if (who.getWhen().getTime() >= oldestReflogTimeMillis) { + if (who.getWhen().getTime() >= reflogExpireOldestReflogTimeMillis) { writer.writeLog( refName, updateIndex, diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java index 14b821a1e..fef5fa57c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java @@ -139,15 +139,9 @@ public class ReftableReader extends Reftable implements AutoCloseable { } /** - * Get the minimum update index for log entries that appear in this - * reftable. - * - * @return the minimum update index for log entries that appear in this - * reftable. This should be 1 higher than the prior reftable's - * {@code maxUpdateIndex} if this table is used in a stack. - * @throws java.io.IOException - * file cannot be read. + * {@inheritDoc} */ + @Override public long minUpdateIndex() throws IOException { if (blockSize == -1) { readFileHeader();