Cache the comparison result between two Git trees / Fail the timed-ou… (

line#375) Related commits: - 47764e9 - 422cec9 - 674d4ab Motivation: - Downgrading jGit from 5.3.0 to 5.2.1 did not help. - We should reuse the `RepositoryCache` for caching the comparison result between two Git trees. - We should fail more timed-out requests fast. Modifications: - Updated jGit from 5.2.1 to 5.3.0. - Moved `server.internal.storage.cache.RepositoryCache` and `CacheableCall` up to `server.internal.storage`. - `GitRepository` now depends on `RepositoryCache` optionally. - Added `CacheableCompareTreesCall` and took advantage of the cache in `GitRepository.findLatestRevision()`. - Removed the forked `DiffScanner` and `DiffEntry`. - Added `FailFastUtil` and used it in most `GitRepository` operations. - Modified `GitRepository.commit0()` to return the `List<DiffEntry>` produced during validation, so that `notifyWatchers()` does not need to compare the trees but reuse what's returned by `commit0()`. - Used `StampedLock` instead of `ReadWriteLock` to make `watch()` completely async. - Added `ReadLockStamp` to implement reentrancy. - Miscellaneous: - Added more test cases to `WatchTest`. - Added `RepositoryCache.get()` and `getIfPresent()` to reduce unsafe casts on the caller side. - Removed unnecessary `try { ... } catch (Exception) { ... }` blocks. - Renamed `GitRepository.toTreeId()` to `toTree()` and changed it to return `RevTree` instead of `ObjectId`. - De-duplicated anonymous `PathEdit` implementations into inner classes. Result: - The comparison result between two Git trees are now stored in `RepositoryCache` which can be configured by a user. - All `GitRepository` operations fail fast when timed out.
trustin · Apr 3, 2019 · 7b7ea60 · 7b7ea60
1 parent 70bbb2f
commit 7b7ea60
Show file tree

Hide file tree

Showing 35 changed files with 831 additions and 969 deletions.
diff --git a/common/src/main/java/com/linecorp/centraldogma/common/Revision.java b/common/src/main/java/com/linecorp/centraldogma/common/Revision.java
@@ -21,6 +21,8 @@
 
 import java.util.regex.Pattern;
 
+import javax.annotation.Nullable;
+
 import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
 import com.fasterxml.jackson.databind.annotation.JsonSerialize;
 
@@ -206,7 +208,7 @@ public int hashCode() {
     }
 
     @Override
-    public boolean equals(Object o) {
+    public boolean equals(@Nullable Object o) {
         if (this == o) {
             return true;
         }

diff --git a/dependencies.yml b/dependencies.yml
@@ -189,7 +189,7 @@ org.eclipse.jetty.alpn:
   alpn-api: { version: '1.1.3.v20160715' }
 
 org.eclipse.jgit:
-  org.eclipse.jgit: { version: '5.2.1.201812262042-r' }
+  org.eclipse.jgit: { version: '5.3.0.201903130848-r' }
 
 org.hibernate.validator:
   hibernate-validator: { version: '6.0.15.Final' }

diff --git a/it/src/test/java/com/linecorp/centraldogma/it/WatchTest.java b/it/src/test/java/com/linecorp/centraldogma/it/WatchTest.java
@@ -63,9 +63,7 @@ public void revertTestFiles() {
 
     @Test
     public void testWatchRepository() throws Exception {
-        final Revision rev1 = client()
-                                  .normalizeRevision(rule.project(), rule.repo1(), Revision.HEAD)
-                                  .join();
+        final Revision rev1 = client().normalizeRevision(rule.project(), rule.repo1(), Revision.HEAD).join();
 
         final CompletableFuture<Revision> future =
                 client().watchRepository(rule.project(), rule.repo1(), rev1, "/**", 3000);
@@ -85,6 +83,55 @@ public void testWatchRepository() throws Exception {
         assertThat(future.get(3, TimeUnit.SECONDS)).isEqualTo(rev2);
     }
 
+    @Test
+    public void testWatchRepositoryImmediateWakeup() throws Exception {
+        final Revision rev1 = client().normalizeRevision(rule.project(), rule.repo1(), Revision.HEAD).join();
+        final Change<JsonNode> change = Change.ofJsonUpsert("/test/test3.json",
+                                                            "[" + System.currentTimeMillis() + ", " +
+                                                            System.nanoTime() + ']');
+
+        final PushResult result = client().push(
+                rule.project(), rule.repo1(), rev1, "Add test3.json", change).join();
+
+        final Revision rev2 = result.revision();
+
+        assertThat(rev2).isEqualTo(rev1.forward(1));
+
+        final CompletableFuture<Revision> future =
+                client().watchRepository(rule.project(), rule.repo1(), rev1, "/**", 3000);
+        assertThat(future.get(3, TimeUnit.SECONDS)).isEqualTo(rev2);
+    }
+
+    @Test
+    public void testWatchRepositoryWithUnrelatedChange() throws Exception {
+        final Revision rev0 = client().normalizeRevision(rule.project(), rule.repo1(), Revision.HEAD).join();
+        final CompletableFuture<Revision> future =
+                client().watchRepository(rule.project(), rule.repo1(), rev0, "/test/test4.json", 3000);
+
+        final Change<JsonNode> change1 = Change.ofJsonUpsert("/test/test3.json",
+                                                             "[" + System.currentTimeMillis() + ", " +
+                                                             System.nanoTime() + ']');
+        final Change<JsonNode> change2 = Change.ofJsonUpsert("/test/test4.json",
+                                                             "[" + System.currentTimeMillis() + ", " +
+                                                             System.nanoTime() + ']');
+
+        final PushResult result1 = client().push(
+                rule.project(), rule.repo1(), rev0, "Add test3.json", change1).join();
+        final Revision rev1 = result1.revision();
+        assertThat(rev1).isEqualTo(rev0.forward(1));
+
+        // Ensure that the watcher is not notified because the path pattern does not match test3.json.
+        assertThatThrownBy(() -> future.get(500, TimeUnit.MILLISECONDS)).isInstanceOf(TimeoutException.class);
+
+        final PushResult result2 = client().push(
+                rule.project(), rule.repo1(), rev1, "Add test4.json", change2).join();
+        final Revision rev2 = result2.revision();
+        assertThat(rev2).isEqualTo(rev1.forward(1));
+
+        // Now it should be notified.
+        assertThat(future.get(3, TimeUnit.SECONDS)).isEqualTo(rev2);
+    }
+
     @Test
     public void testWatchRepositoryTimeout() {
         final Revision rev = client().watchRepository(

diff --git a/.../linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryBenchmark.java b/.../linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryBenchmark.java
@@ -57,7 +57,7 @@ public class GitRepositoryBenchmark {
     public void init() throws Exception {
         repoDir = Files.createTempDirectory("jmh-gitrepository.").toFile();
         repo = new GitRepository(mock(Project.class), repoDir, format, ForkJoinPool.commonPool(),
-                                 System.currentTimeMillis(), AUTHOR);
+                                 System.currentTimeMillis(), AUTHOR, null);
         currentRevision = 1;
 
         for (int i = 0; i < previousCommits; i++) {

diff --git a/server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaBuilder.java b/server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaBuilder.java
@@ -19,7 +19,7 @@
 import static com.linecorp.centraldogma.server.auth.AuthConfig.DEFAULT_SESSION_CACHE_SPEC;
 import static com.linecorp.centraldogma.server.auth.AuthConfig.DEFAULT_SESSION_TIMEOUT_MILLIS;
 import static com.linecorp.centraldogma.server.auth.AuthConfig.DEFAULT_SESSION_VALIDATION_SCHEDULE;
-import static com.linecorp.centraldogma.server.internal.storage.repository.cache.RepositoryCache.validateCacheSpec;
+import static com.linecorp.centraldogma.server.internal.storage.repository.RepositoryCache.validateCacheSpec;
 import static java.util.Objects.requireNonNull;
 
 import java.io.File;

diff --git a/server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaConfig.java b/server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaConfig.java
@@ -23,7 +23,7 @@
 import static com.linecorp.centraldogma.server.CentralDogmaBuilder.DEFAULT_NUM_MIRRORING_THREADS;
 import static com.linecorp.centraldogma.server.CentralDogmaBuilder.DEFAULT_NUM_REPOSITORY_WORKERS;
 import static com.linecorp.centraldogma.server.CentralDogmaBuilder.DEFAULT_REPOSITORY_CACHE_SPEC;
-import static com.linecorp.centraldogma.server.internal.storage.repository.cache.RepositoryCache.validateCacheSpec;
+import static com.linecorp.centraldogma.server.internal.storage.repository.RepositoryCache.validateCacheSpec;
 import static java.util.Objects.requireNonNull;
 
 import java.io.File;

diff --git a/server/src/main/java/com/linecorp/centraldogma/server/auth/AuthConfig.java b/server/src/main/java/com/linecorp/centraldogma/server/auth/AuthConfig.java
@@ -17,7 +17,7 @@
 
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.linecorp.centraldogma.server.internal.storage.repository.cache.RepositoryCache.validateCacheSpec;
+import static com.linecorp.centraldogma.server.internal.storage.repository.RepositoryCache.validateCacheSpec;
 import static java.util.Objects.requireNonNull;
 
 import java.text.ParseException;

diff --git a/...c/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java b/...c/main/java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProject.java
@@ -51,9 +51,9 @@
 import com.linecorp.centraldogma.server.internal.storage.repository.DefaultMetaRepository;
 import com.linecorp.centraldogma.server.internal.storage.repository.MetaRepository;
 import com.linecorp.centraldogma.server.internal.storage.repository.Repository;
+import com.linecorp.centraldogma.server.internal.storage.repository.RepositoryCache;
 import com.linecorp.centraldogma.server.internal.storage.repository.RepositoryManager;
 import com.linecorp.centraldogma.server.internal.storage.repository.cache.CachingRepositoryManager;
-import com.linecorp.centraldogma.server.internal.storage.repository.cache.RepositoryCache;
 import com.linecorp.centraldogma.server.internal.storage.repository.git.GitRepositoryManager;
 
 class DefaultProject implements Project {
@@ -120,7 +120,7 @@ class DefaultProject implements Project {
     private RepositoryManager newRepoManager(File rootDir, Executor repositoryWorker,
                                              @Nullable RepositoryCache cache) {
         // Enable caching if 'cache' is not null.
-        final GitRepositoryManager gitRepos = new GitRepositoryManager(this, rootDir, repositoryWorker);
+        final GitRepositoryManager gitRepos = new GitRepositoryManager(this, rootDir, repositoryWorker, cache);
         return cache == null ? gitRepos : new CachingRepositoryManager(gitRepos, cache);
     }
 

diff --git a/...java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProjectManager.java b/...java/com/linecorp/centraldogma/server/internal/storage/project/DefaultProjectManager.java
@@ -31,7 +31,7 @@
 import com.linecorp.centraldogma.common.ProjectExistsException;
 import com.linecorp.centraldogma.common.ProjectNotFoundException;
 import com.linecorp.centraldogma.server.internal.storage.DirectoryBasedStorageManager;
-import com.linecorp.centraldogma.server.internal.storage.repository.cache.RepositoryCache;
+import com.linecorp.centraldogma.server.internal.storage.repository.RepositoryCache;
 
 public class DefaultProjectManager extends DirectoryBasedStorageManager<Project> implements ProjectManager {
 

diff --git a/...orage/repository/cache/CacheableCall.java → ...nal/storage/repository/CacheableCall.java b/...orage/repository/cache/CacheableCall.java → ...nal/storage/repository/CacheableCall.java
@@ -14,28 +14,30 @@
  * under the License.
  */
 
-package com.linecorp.centraldogma.server.internal.storage.repository.cache;
+package com.linecorp.centraldogma.server.internal.storage.repository;
 
 import static java.util.Objects.requireNonNull;
 
 import java.util.concurrent.CompletableFuture;
 
 import com.google.common.base.MoreObjects;
 
-import com.linecorp.centraldogma.server.internal.storage.repository.Repository;
-
 // XXX(trustin): Consider using reflection or AOP so that it takes less effort to add more call types.
-abstract class CacheableCall<T> {
+public abstract class CacheableCall<T> {
 
     final Repository repo;
 
-    CacheableCall(Repository repo) {
+    protected CacheableCall(Repository repo) {
         this.repo = requireNonNull(repo, "repo");
     }
 
-    abstract int weigh(T value);
+    public final Repository repo() {
+        return repo;
+    }
+
+    protected abstract int weigh(T value);
 
-    abstract CompletableFuture<T> execute();
+    public abstract CompletableFuture<T> execute();
 
     @Override
     public int hashCode() {
@@ -69,5 +71,5 @@ public final String toString() {
         return helper.toString();
     }
 
-    abstract void toString(MoreObjects.ToStringHelper helper);
+    protected abstract void toString(MoreObjects.ToStringHelper helper);
 }
diff --git a/...age/repository/cache/RepositoryCache.java → ...l/storage/repository/RepositoryCache.java b/...age/repository/cache/RepositoryCache.java → ...l/storage/repository/RepositoryCache.java
@@ -14,10 +14,12 @@
  * under the License.
  */
 
-package com.linecorp.centraldogma.server.internal.storage.repository.cache;
+package com.linecorp.centraldogma.server.internal.storage.repository;
 
 import static java.util.Objects.requireNonNull;
 
+import java.util.concurrent.CompletableFuture;
+
 import javax.annotation.Nullable;
 
 import org.slf4j.Logger;
@@ -49,7 +51,7 @@ public static String validateCacheSpec(@Nullable String cacheSpec) {
     }
 
     @SuppressWarnings("rawtypes")
-    final AsyncLoadingCache<CacheableCall, Object> cache;
+    private final AsyncLoadingCache<CacheableCall, Object> cache;
     private final String cacheSpec;
 
     @SuppressWarnings({ "rawtypes", "unchecked" })
@@ -67,6 +69,21 @@ public RepositoryCache(String cacheSpec) {
                        });
     }
 
+    public <T> CompletableFuture<T> get(CacheableCall<T> call) {
+        requireNonNull(call, "call");
+        @SuppressWarnings("unchecked")
+        final CompletableFuture<T> f = (CompletableFuture<T>) cache.get(call);
+        return f;
+    }
+
+    @Nullable
+    public <T> CompletableFuture<T> getIfPresent(CacheableCall<T> call) {
+        requireNonNull(call, "call");
+        @SuppressWarnings("unchecked")
+        final CompletableFuture<T> f = (CompletableFuture<T>) cache.getIfPresent(call);
+        return f;
+    }
+
     public CacheStats stats() {
         return cache.synchronous().stats();
     }

diff --git a/...com/linecorp/centraldogma/server/internal/storage/repository/cache/CacheableFindCall.java b/...com/linecorp/centraldogma/server/internal/storage/repository/cache/CacheableFindCall.java
@@ -26,6 +26,7 @@
 
 import com.linecorp.centraldogma.common.Entry;
 import com.linecorp.centraldogma.common.Revision;
+import com.linecorp.centraldogma.server.internal.storage.repository.CacheableCall;
 import com.linecorp.centraldogma.server.internal.storage.repository.FindOption;
 import com.linecorp.centraldogma.server.internal.storage.repository.Repository;
 
@@ -49,7 +50,7 @@ final class CacheableFindCall extends CacheableCall<Map<String, Entry<?>>> {
     }
 
     @Override
-    int weigh(Map<String, Entry<?>> value) {
+    protected int weigh(Map<String, Entry<?>> value) {
         int weight = 0;
         weight += pathPattern.length();
         weight += options.size();
@@ -63,8 +64,8 @@ int weigh(Map<String, Entry<?>> value) {
     }
 
     @Override
-    CompletableFuture<Map<String, Entry<?>>> execute() {
-        return repo.find(revision, pathPattern, options);
+    public CompletableFuture<Map<String, Entry<?>>> execute() {
+        return repo().find(revision, pathPattern, options);
     }
 
     @Override
@@ -85,7 +86,7 @@ public boolean equals(Object o) {
     }
 
     @Override
-    void toString(ToStringHelper helper) {
+    protected void toString(ToStringHelper helper) {
         helper.add("revision", revision)
               .add("pathPattern", pathPattern)
               .add("options", options);

diff --git a/...orp/centraldogma/server/internal/storage/repository/cache/CacheableFindLatestRevCall.java b/...orp/centraldogma/server/internal/storage/repository/cache/CacheableFindLatestRevCall.java
@@ -24,6 +24,7 @@
 import com.google.common.base.MoreObjects.ToStringHelper;
 
 import com.linecorp.centraldogma.common.Revision;
+import com.linecorp.centraldogma.server.internal.storage.repository.CacheableCall;
 import com.linecorp.centraldogma.server.internal.storage.repository.Repository;
 
 final class CacheableFindLatestRevCall extends CacheableCall<Revision> {
@@ -49,13 +50,13 @@ final class CacheableFindLatestRevCall extends CacheableCall<Revision> {
     }
 
     @Override
-    int weigh(Revision value) {
+    protected int weigh(Revision value) {
         return pathPattern.length();
     }
 
     @Override
-    CompletableFuture<Revision> execute() {
-        return repo.findLatestRevision(lastKnownRevision, pathPattern).thenApply(e -> firstNonNull(e, EMPTY));
+    public CompletableFuture<Revision> execute() {
+        return repo().findLatestRevision(lastKnownRevision, pathPattern).thenApply(e -> firstNonNull(e, EMPTY));
     }
 
     @Override
@@ -76,7 +77,7 @@ public boolean equals(Object o) {
     }
 
     @Override
-    void toString(ToStringHelper helper) {
+    protected void toString(ToStringHelper helper) {
         helper.add("lastKnownRevision", lastKnownRevision)
               .add("headRevision", headRevision)
               .add("pathPattern", pathPattern);

diff --git a/.../linecorp/centraldogma/server/internal/storage/repository/cache/CacheableHistoryCall.java b/.../linecorp/centraldogma/server/internal/storage/repository/cache/CacheableHistoryCall.java
@@ -26,6 +26,7 @@
 
 import com.linecorp.centraldogma.common.Commit;
 import com.linecorp.centraldogma.common.Revision;
+import com.linecorp.centraldogma.server.internal.storage.repository.CacheableCall;
 import com.linecorp.centraldogma.server.internal.storage.repository.Repository;
 
 final class CacheableHistoryCall extends CacheableCall<List<Commit>> {
@@ -51,7 +52,7 @@ final class CacheableHistoryCall extends CacheableCall<List<Commit>> {
     }
 
     @Override
-    int weigh(List<Commit> value) {
+    protected int weigh(List<Commit> value) {
         int weight = 0;
         weight += pathPattern.length();
         for (Commit c : value) {
@@ -62,8 +63,8 @@ int weigh(List<Commit> value) {
     }
 
     @Override
-    CompletableFuture<List<Commit>> execute() {
-        return repo.history(from, to, pathPattern, maxCommits);
+    public CompletableFuture<List<Commit>> execute() {
+        return repo().history(from, to, pathPattern, maxCommits);
     }
 
     @Override
@@ -85,7 +86,7 @@ public boolean equals(Object o) {
     }
 
     @Override
-    void toString(ToStringHelper helper) {
+    protected void toString(ToStringHelper helper) {
         helper.add("from", from)
               .add("to", to)
               .add("pathPattern", pathPattern)

diff --git a/...necorp/centraldogma/server/internal/storage/repository/cache/CacheableMergeQueryCall.java b/...necorp/centraldogma/server/internal/storage/repository/cache/CacheableMergeQueryCall.java
@@ -32,6 +32,7 @@
 import com.linecorp.centraldogma.common.MergeSource;
 import com.linecorp.centraldogma.common.MergedEntry;
 import com.linecorp.centraldogma.common.Revision;
+import com.linecorp.centraldogma.server.internal.storage.repository.CacheableCall;
 import com.linecorp.centraldogma.server.internal.storage.repository.Repository;
 
 final class CacheableMergeQueryCall extends CacheableCall<MergedEntry<?>> {
@@ -56,7 +57,7 @@ final class CacheableMergeQueryCall extends CacheableCall<MergedEntry<?>> {
     }
 
     @Override
-    int weigh(MergedEntry<?> value) {
+    protected int weigh(MergedEntry<?> value) {
         int weight = 0;
         final List<MergeSource> mergeSources = query.mergeSources();
         weight += mergeSources.size();
@@ -75,7 +76,7 @@ int weigh(MergedEntry<?> value) {
     }
 
     @Override
-    CompletableFuture<MergedEntry<?>> execute() {
+    public CompletableFuture<MergedEntry<?>> execute() {
         checkState(computedValue != null, "computedValue is not set yet.");
         return CompletableFuture.completedFuture(computedValue);
     }
@@ -102,7 +103,7 @@ public boolean equals(Object o) {
     }
 
     @Override
-    void toString(ToStringHelper helper) {
+    protected void toString(ToStringHelper helper) {
         helper.add("revision", revision)
               .add("query", query);
     }