]> source.dussan.org Git - jgit.git/commitdiff
Simplify ReftableCompactor 81/156281/6
authorHan-Wen Nienhuys <hanwen@google.com>
Tue, 21 Jan 2020 18:06:30 +0000 (19:06 +0100)
committerMatthias Sohn <matthias.sohn@sap.com>
Wed, 12 Feb 2020 22:08:20 +0000 (23:08 +0100)
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 <hanwen@google.com>
org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableCompactorTest.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/MergedReftable.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/Reftable.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableCompactor.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java

index 9a7f2409517c3a1d95ed13d15b19d6223a24d3c6..0947dd210850abaa8964d9f350e08e8426a32ba0 100644 (file)
@@ -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<ReftableReader> readers = new ArrayList<>();
+                       readers.add(read(inTab));
+                       compactor.addAll(readers);
                        compactor.compact();
                        outTab = outBuf.toByteArray();
                }
index cc52bfb719945a373dc92f3ddfc7203afc7b3fab..7abd1fb2823e4a7f8ea7e80c32f74de0b59109c7 100644 (file)
@@ -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<ReftableReader> compactMe = new ArrayList<>();
index 0144453cd77360997b2a454388b861f173355cbc..d272d09f008dd0df2dbb6bfe0d4afa6e80f7f176 100644 (file)
@@ -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 {
index 8a8a14375a16963056f7151d28531f12e7abf59e..a7e8252b48b8a0d402af631f38da799b5504020b 100644 (file)
@@ -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.
         *
index c73c245be523f8fd0e5310d3988c7c77fa3f4691..a586d42de6c7d6a3a4c89c16ce9b972672b9608d 100644 (file)
@@ -61,22 +61,20 @@ import org.eclipse.jgit.lib.ReflogEntry;
  * to shadow any lower reftable that may have the reference present.
  * <p>
  * 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<ReftableReader> 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.
-        * <p>
-        * 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<ReftableReader> readers) throws IOException {
                for (ReftableReader r : readers) {
                        tables.add(r);
-                       adjustUpdateIndexes(r);
-               }
-       }
-
-       /**
-        * Try to add this reader at the bottom of the stack.
-        * <p>
-        * 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,
index 14b821a1ed03472e90ff189f212cbcafbe30989a..fef5fa57c6b1027b97e1e0799e6f6f50c397d86a 100644 (file)
@@ -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();