Merge branch 'stable-3.6' into stable-3.7

* stable-3.6:
  Set version to 3.6.4-SNAPSHOT
  Set version to 3.6.3
  Set version to 3.5.5-SNAPSHOT
  Set version to 3.5.4
  CopyApprovalsIT: remove unnecessary comments
  Log progress of the copy-approvals program
  copy-approvals: do not lock loose refs when executing BatchRefUpdate
  copy-approvals: multi-threaded, slice based
  Update git submodules
  Set version to 3.4.9-SNAPSHOT
  Set version to 3.4.8
  Fix bulk loading of entries with PassthroughLoadingCache
  Update git submodules
  Set version to 3.4.8-SNAPSHOT
  Set version to 3.4.7
  Introduce a PassthroughLoadingCache for disabled caches
  Export commons:lang3 dependency to plugin API
  copy-approvals: continue when there are corrupt meta-refs in a project
  copy-approvals: don't stop when it fails on one project
  Fix CopyApprovalsIT.multipleProjects: make sure to use the secondProject

Release-Notes: skip
Forward-Compatible: checked
Change-Id: I10ddbe0dd34f995245c7bae8fbd938a5535911f2

7 files changed, 397 insertions, 7 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 1c262324ef..e880314064 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -870,8 +870,14 @@ Default is 1024 for most caches, except:
 * `"plugin_resources"`: default is 2m (2 MiB of memory)
 
 +
-If set to 0 the cache is disabled. Entries are removed immediately
-after being stored by the cache. This is primarily useful for testing.
+If set to 0 the cache is disabled; entries are loaded but not stored
+in-memory.
++
+**NOTE**: When the cache is disabled, there is no locking when accessing
+the same key/value, and therefore multiple threads may
+load the same value concurrently with a higher memory footprint.
+To keep a minimum caching and avoid concurrent loading of the same
+key/value, set `memoryLimit` to `1` and `maxAge` to `1`.
 
 [[cache.name.expireFromMemoryAfterAccess]]cache.<name>.expireFromMemoryAfterAccess::
 +
diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
index 3b104dd244..28a2ede3a7 100644
--- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
+++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java
@@ -55,14 +55,15 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory {
 
   @Override
   public <K, V> LoadingCache<K, V> build(CacheDef<K, V> def, CacheLoader<K, V> loader) {
-    return CaffeinatedGuava.build(create(def), loader);
+    return cacheMaximumWeight(def) == 0
+        ? new PassthroughLoadingCache<>(loader)
+        : CaffeinatedGuava.build(create(def), loader);
   }
 
   private <K, V> Caffeine<K, V> create(CacheDef<K, V> def) {
     Caffeine<K, V> builder = newCacheBuilder();
     builder.recordStats();
-    builder.maximumWeight(
-        cfg.getLong("cache", def.configKey(), "memoryLimit", def.maximumWeight()));
+    builder.maximumWeight(cacheMaximumWeight(def));
     builder = builder.removalListener(newRemovalListener(def.name()));
     builder.weigher(newWeigher(def.weigher()));
 
@@ -109,6 +110,10 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory {
     return builder;
   }
 
+  private <K, V> long cacheMaximumWeight(CacheDef<K, V> def) {
+    return cfg.getLong("cache", def.configKey(), "memoryLimit", def.maximumWeight());
+  }
+
   private static long toSeconds(@Nullable Duration duration) {
     return duration != null ? duration.getSeconds() : 0;
   }
diff --git a/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java b/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java
new file mode 100644
index 0000000000..ded21aae5b
--- /dev/null
+++ b/java/com/google/gerrit/server/cache/mem/PassthroughLoadingCache.java
@@ -0,0 +1,141 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed 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 com.google.gerrit.server.cache.mem;
+
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.CacheLoader.UnsupportedLoadingOperationException;
+import com.google.common.cache.CacheStats;
+import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.common.Nullable;
+import java.util.Map;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ExecutionException;
+
+/** Implementation of a NOOP cache that just passes all gets to the loader */
+public class PassthroughLoadingCache<K, V> implements LoadingCache<K, V> {
+
+  private final CacheLoader<? super K, V> cacheLoader;
+
+  public PassthroughLoadingCache(CacheLoader<? super K, V> cacheLoader) {
+    this.cacheLoader = cacheLoader;
+  }
+
+  @Override
+  public @Nullable V getIfPresent(Object key) {
+    return null;
+  }
+
+  @Override
+  public V get(K key, Callable<? extends V> loader) throws ExecutionException {
+    try {
+      return loader.call();
+    } catch (Exception e) {
+      throw new ExecutionException(e);
+    }
+  }
+
+  @Override
+  public ImmutableMap<K, V> getAllPresent(Iterable<?> keys) {
+    return ImmutableMap.of();
+  }
+
+  @Override
+  public void put(K key, V value) {}
+
+  @Override
+  public void putAll(Map<? extends K, ? extends V> m) {}
+
+  @Override
+  public void invalidate(Object key) {}
+
+  @Override
+  public void invalidateAll(Iterable<?> keys) {}
+
+  @Override
+  public void invalidateAll() {}
+
+  @Override
+  public long size() {
+    return 0;
+  }
+
+  @Override
+  public CacheStats stats() {
+    return new CacheStats(0, 0, 0, 0, 0, 0);
+  }
+
+  @Override
+  public void cleanUp() {}
+
+  @Override
+  public V get(K key) throws ExecutionException {
+    try {
+      return cacheLoader.load(key);
+    } catch (Exception e) {
+      throw new ExecutionException(e);
+    }
+  }
+
+  @Override
+  public V getUnchecked(K key) {
+    try {
+      return cacheLoader.load(key);
+    } catch (Exception e) {
+      throw new IllegalStateException(e);
+    }
+  }
+
+  @Override
+  public ImmutableMap<K, V> getAll(Iterable<? extends K> keys) throws ExecutionException {
+    try {
+      try {
+        return getAllBulk(keys);
+      } catch (UnsupportedLoadingOperationException e) {
+        return getAllIndividually(keys);
+      }
+    } catch (Exception e) {
+      throw new ExecutionException(e);
+    }
+  }
+
+  private ImmutableMap<K, V> getAllIndividually(Iterable<? extends K> keys) throws Exception {
+    ImmutableMap.Builder<K, V> builder = ImmutableMap.builder();
+    for (K k : keys) {
+      builder.put(k, cacheLoader.load(k));
+    }
+    return builder.build();
+  }
+
+  @SuppressWarnings("unchecked")
+  private ImmutableMap<K, V> getAllBulk(Iterable<? extends K> keys) throws Exception {
+    return (ImmutableMap<K, V>) ImmutableMap.copyOf(cacheLoader.loadAll(keys));
+  }
+
+  @Override
+  public V apply(K key) {
+    return getUnchecked(key);
+  }
+
+  @Override
+  public void refresh(K key) {}
+
+  @Override
+  public ConcurrentMap<K, V> asMap() {
+    return new ConcurrentHashMap<>();
+  }
+}
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
index ad1f4c5155..c9280f1b61 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -310,6 +310,12 @@ public class NoteDbUpdateManager implements AutoCloseable {
     }
   }
 
+  public BatchRefUpdate prepare() throws IOException {
+    checkNotExecuted();
+    stage();
+    return prepare(changeRepo, false, pushCert);
+  }
+
   @Nullable
   public BatchRefUpdate execute() throws IOException {
     return execute(false);
@@ -365,7 +371,7 @@ public class NoteDbUpdateManager implements AutoCloseable {
                 cu -> cu.getAttentionSetUpdates().stream()));
   }
 
-  private BatchRefUpdate execute(OpenRepo or, boolean dryrun, @Nullable PushCertificate pushCert)
+  private BatchRefUpdate prepare(OpenRepo or, boolean dryrun, @Nullable PushCertificate pushCert)
       throws IOException {
     if (or == null || or.cmds.isEmpty()) {
       return null;
@@ -394,7 +400,13 @@ public class NoteDbUpdateManager implements AutoCloseable {
       bru = listener.beforeUpdateRefs(bru);
     }
 
-    if (!dryrun) {
+    return bru;
+  }
+
+  private BatchRefUpdate execute(OpenRepo or, boolean dryrun, @Nullable PushCertificate pushCert)
+      throws IOException {
+    BatchRefUpdate bru = prepare(or, dryrun, pushCert);
+    if (bru != null && !dryrun) {
       RefUpdateUtil.executeChecked(bru, or.rw);
     }
     return bru;
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 32529f7ca1..db4509a077 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -453,6 +453,11 @@ public class BatchUpdate implements AutoCloseable {
     execute(ImmutableList.of(this), ImmutableList.of(), false);
   }
 
+  public BatchRefUpdate prepareRefUpdates() throws Exception {
+    ChangesHandle handle = executeChangeOps(ImmutableList.of(), false);
+    return handle.prepare();
+  }
+
   public boolean isExecuted() {
     return executed;
   }
@@ -639,6 +644,10 @@ public class BatchUpdate implements AutoCloseable {
       checkArgument(old == null, "result for change %s already set: %s", id, old);
     }
 
+    public BatchRefUpdate prepare() throws IOException {
+      return manager.prepare();
+    }
+
     void execute() throws IOException {
       BatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
       BatchUpdate.this.executed = manager.isExecuted();
diff --git a/javatests/com/google/gerrit/server/cache/mem/BUILD b/javatests/com/google/gerrit/server/cache/mem/BUILD
new file mode 100644
index 0000000000..a263c7b315
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cache/mem/BUILD
@@ -0,0 +1,14 @@
+load("//tools/bzl:junit.bzl", "junit_tests")
+
+junit_tests(
+    name = "tests",
+    srcs = glob(["*Test.java"]),
+    deps = [
+        "//java/com/google/gerrit/server",
+        "//java/com/google/gerrit/server/cache/mem",
+        "//lib:jgit",
+        "//lib:junit",
+        "//lib/guice",
+        "//lib/truth",
+    ],
+)
diff --git a/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
new file mode 100644
index 0000000000..595846571f
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactoryTest.java
@@ -0,0 +1,203 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed 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 com.google.gerrit.server.cache.mem;
+
+import static com.google.common.base.Functions.identity;
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.common.cache.Weigher;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.server.cache.CacheDef;
+import com.google.inject.TypeLiteral;
+import java.time.Duration;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.BrokenBarrierException;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+public class DefaultMemoryCacheFactoryTest {
+
+  private static final String TEST_CACHE = "test-cache";
+  private static final long TEST_TIMEOUT_SEC = 1;
+  private static final int TEST_CACHE_KEY = 1;
+
+  private DefaultMemoryCacheFactory memoryCacheFactory;
+  private Config memoryCacheConfig;
+  private ScheduledExecutorService executor;
+  private CyclicBarrier cacheGetStarted;
+  private CyclicBarrier cacheGetCompleted;
+
+  @Before
+  public void setUp() {
+    memoryCacheConfig = new Config();
+    memoryCacheFactory = new DefaultMemoryCacheFactory(memoryCacheConfig, null);
+    executor = Executors.newScheduledThreadPool(1);
+    cacheGetStarted = new CyclicBarrier(2);
+    cacheGetCompleted = new CyclicBarrier(2);
+  }
+
+  @Test
+  public void shouldNotBlockEvictionsWhenCacheIsDisabledByDefault() throws Exception {
+    LoadingCache<Integer, Integer> disabledCache =
+        memoryCacheFactory.build(newCacheDef(0), newCacheLoader(identity()));
+
+    assertCacheEvictionIsNotBlocking(disabledCache);
+  }
+
+  @Test
+  public void shouldNotBlockEvictionsWhenCacheIsDisabledByConfiguration() throws Exception {
+    memoryCacheConfig.setInt("cache", TEST_CACHE, "memoryLimit", 0);
+    LoadingCache<Integer, Integer> disabledCache =
+        memoryCacheFactory.build(newCacheDef(1), newCacheLoader(identity()));
+
+    assertCacheEvictionIsNotBlocking(disabledCache);
+  }
+
+  @Test
+  public void shouldBlockEvictionsWhenCacheIsEnabled() throws Exception {
+    LoadingCache<Integer, Integer> cache =
+        memoryCacheFactory.build(newCacheDef(1), newCacheLoader(identity()));
+
+    ScheduledFuture<Integer> cacheValue =
+        executor.schedule(() -> cache.getUnchecked(TEST_CACHE_KEY), 0, TimeUnit.SECONDS);
+
+    cacheGetStarted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+    cache.invalidate(TEST_CACHE_KEY);
+
+    assertThat(cacheValue.isDone()).isTrue();
+    assertThat(cacheValue.get()).isEqualTo(TEST_CACHE_KEY);
+  }
+
+  @Test
+  public void shouldLoadAllKeysWithDisabledCache() throws Exception {
+    LoadingCache<Integer, Integer> disabledCache =
+        memoryCacheFactory.build(newCacheDef(0), newCacheLoader(identity()));
+
+    List<Integer> keys = Arrays.asList(1, 2);
+    ImmutableMap<Integer, Integer> entries = disabledCache.getAll(keys);
+
+    assertThat(entries).containsExactly(1, 1, 2, 2);
+  }
+
+  private void assertCacheEvictionIsNotBlocking(LoadingCache<Integer, Integer> disabledCache)
+      throws InterruptedException, BrokenBarrierException, TimeoutException, ExecutionException {
+    ScheduledFuture<Integer> cacheValue =
+        executor.schedule(() -> disabledCache.getUnchecked(TEST_CACHE_KEY), 0, TimeUnit.SECONDS);
+    cacheGetStarted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+    disabledCache.invalidate(TEST_CACHE_KEY);
+
+    // The invalidate did not wait for the cache loader to finish, therefore the cacheValue isn't
+    // done yet
+    assertThat(cacheValue.isDone()).isFalse();
+
+    // The cache loader completes after the invalidation
+    cacheGetCompleted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+    assertThat(cacheValue.get()).isEqualTo(TEST_CACHE_KEY);
+  }
+
+  private CacheLoader<Integer, Integer> newCacheLoader(Function<Integer, Integer> loadFunc) {
+    return new CacheLoader<>() {
+
+      @Override
+      public Integer load(Integer n) throws Exception {
+        Integer v = 0;
+        try {
+          cacheGetStarted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+          v = loadFunc.apply(n);
+          cacheGetCompleted.await(TEST_TIMEOUT_SEC, TimeUnit.SECONDS);
+        } catch (TimeoutException | BrokenBarrierException e) {
+          // Just continue
+        }
+        return v;
+      }
+
+      @Override
+      public Map<Integer, Integer> loadAll(Iterable<? extends Integer> keys) throws Exception {
+        return StreamSupport.stream(keys.spliterator(), false)
+            .collect(Collectors.toMap(identity(), identity()));
+      }
+    };
+  }
+
+  private CacheDef<Integer, Integer> newCacheDef(long maximumWeight) {
+    return new CacheDef<>() {
+
+      @Override
+      public String name() {
+        return TEST_CACHE;
+      }
+
+      @Override
+      public String configKey() {
+        return TEST_CACHE;
+      }
+
+      @Override
+      public TypeLiteral<Integer> keyType() {
+        return null;
+      }
+
+      @Override
+      public TypeLiteral<Integer> valueType() {
+        return null;
+      }
+
+      @Override
+      public long maximumWeight() {
+        return maximumWeight;
+      }
+
+      @Override
+      public Duration expireAfterWrite() {
+        return null;
+      }
+
+      @Override
+      public Duration expireFromMemoryAfterAccess() {
+        return null;
+      }
+
+      @Override
+      public Duration refreshAfterWrite() {
+        return null;
+      }
+
+      @Override
+      public Weigher<Integer, Integer> weigher() {
+        return null;
+      }
+
+      @Override
+      public CacheLoader<Integer, Integer> loader() {
+        return null;
+      }
+    };
+  }
+}