Merge branch 'stable-3.5' into stable-3.6
* stable-3.5:
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
Change-Id: Ie771563ec237a96f591c2e20772d07a9d06c502d
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 5682a15..b1949e6 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -870,8 +870,14 @@
* `"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 3b104dd..28a2ede 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 @@
@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 @@
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 0000000..ded21aa
--- /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 94e11c8..b52671b 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -306,6 +306,12 @@
}
}
+ public BatchRefUpdate prepare() throws IOException {
+ checkNotExecuted();
+ stage();
+ return prepare(changeRepo, false, pushCert);
+ }
+
@Nullable
public BatchRefUpdate execute() throws IOException {
return execute(false);
@@ -353,7 +359,7 @@
}
}
- 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;
@@ -382,7 +388,13 @@
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 f26882a..77f3de4 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -434,6 +434,11 @@
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;
}
@@ -610,6 +615,10 @@
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 0000000..a263c7b
--- /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 0000000..5958465
--- /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;
+ }
+ };
+ }
+}
diff --git a/modules/jgit b/modules/jgit
index d013761..cb90ed0 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit d01376106af8800017ac3c08d7c7ac1fd5ccc9ee
+Subproject commit cb90ed08526bd51f04e5d72e3ba3cf5bd30c11e4
