Merge branch 'stable-3.5' into stable-3.6
* stable-3.5:
Allow specify either avgKeySize or avgValueSize as command parameters
Fix avg value and key size calculation when auto-adjusting caches
Apply Flogger fixes from Ia4e5a3c513
Change-Id: Ic4238ee6137910f946ae8f0b669aa68d11482bdb
diff --git a/BUILD b/BUILD
index dc34ede..a782456 100644
--- a/BUILD
+++ b/BUILD
@@ -27,11 +27,11 @@
"@chronicle-threads//jar",
"@chronicle-values//jar",
"@chronicle-wire//jar",
+ "@commons-lang3//jar",
"@dev-jna//jar",
"@error-prone-annotations//jar",
"@javapoet//jar",
"@jna-platform//jar",
- "@commons-lang3//jar",
],
)
@@ -41,30 +41,31 @@
["src/test/java/**/*Test.java"],
),
visibility = ["//visibility:public"],
- deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
+ deps = [
":cache-chroniclemap__plugin",
- "@chronicle-bytes//jar",
":chroniclemap-test-lib",
+ "@chronicle-bytes//jar",
],
)
-acceptance_tests(
- srcs = glob(["src/test/java/**/*IT.java"]),
- group = "server_cache",
- labels = ["server"],
- vm_args = ["-Xmx4g"],
+[junit_tests(
+ name = f[:f.index(".")].replace("/", "_"),
+ srcs = [f],
+ tags = ["server"],
deps = [
":cache-chroniclemap__plugin",
":chroniclemap-test-lib",
"//java/com/google/gerrit/server/cache/h2",
"//java/com/google/gerrit/server/cache/serialize",
"//proto:cache_java_proto",
+ "@chronicle-bytes//jar",
],
-)
+) for f in glob(["src/test/java/**/*IT.java"])]
java_library(
name = "chroniclemap-test-lib",
testonly = True,
srcs = ["src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/TestPersistentCacheDef.java"],
- deps = PLUGIN_DEPS,
+ exports = PLUGIN_DEPS + PLUGIN_TEST_DEPS,
+ deps = PLUGIN_DEPS + PLUGIN_TEST_DEPS,
)
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
index 0730380..35a1cc3 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheKeysIndex.java
@@ -94,7 +94,7 @@
private final Counter0 persistFailures;
private Metrics(MetricMaker metricMaker, String name) {
- String sanitizedName = metricMaker.sanitizeMetricName(name);
+ String sanitizedName = CacheNameSanitizer.sanitize(metricMaker, name);
indexSize =
metricMaker.newCallbackMetric(
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizer.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizer.java
new file mode 100644
index 0000000..212aae3
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizer.java
@@ -0,0 +1,41 @@
+// Copyright (C) 2023 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.googlesource.gerrit.modules.cache.chroniclemap;
+
+import com.google.gerrit.metrics.MetricMaker;
+import java.util.regex.Pattern;
+
+class CacheNameSanitizer {
+ private static final Pattern METRIC_NAME_PATTERN =
+ Pattern.compile("[a-zA-Z0-9_-]+([a-zA-Z0-9_-]+)*");
+
+ /**
+ * Detect if <code>cacheName</code> contains only metric name allowed characters (that matches
+ * {@link #METRIC_NAME_PATTERN}). Note that `/` regardless of being allowed in metric names is
+ * omitted as it denotes the sub-metric and cache name should not be treated as metric /
+ * sub-metric. Typical persistent cache name (e.g. `diff_summary`) adheres to it therefore it is
+ * returned without any modification. In all other cases call to {@link
+ * MetricMaker#sanitizeMetricName(String)} is performed so that name is sanitized. This way
+ * sanitization stays backward compatible but also non-typical cases are handled.
+ */
+ static String sanitize(MetricMaker metricMaker, String cacheName) {
+ if (METRIC_NAME_PATTERN.matcher(cacheName).matches()) {
+ return cacheName;
+ }
+ return metricMaker.sanitizeMetricName(cacheName);
+ }
+
+ private CacheNameSanitizer() {}
+}
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
index e2c5afe..37d2845 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheConfig.java
@@ -264,7 +264,6 @@
static final ImmutableMap<String, DefaultConfig> defaultMap =
new ImmutableMap.Builder<String, DefaultConfig>()
.put("accounts", DefaultConfig.create(30, 256, 1000, 1))
- .put("approvals", DefaultConfig.create(103, 365, 1000, 3))
.put("change_kind", DefaultConfig.create(59, 26, 1000, 1))
.put("change_notes", DefaultConfig.create(36, 10240, 1000, 3))
.put("comment_context", DefaultConfig.create(80, 662, 2000, 3))
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
index e4474b0..27be43f 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheFactory.java
@@ -21,7 +21,6 @@
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.metrics.MetricMaker;
-import com.google.gerrit.server.cache.CacheBackend;
import com.google.gerrit.server.cache.MemoryCacheFactory;
import com.google.gerrit.server.cache.PersistentCacheBaseFactory;
import com.google.gerrit.server.cache.PersistentCacheDef;
@@ -89,24 +88,20 @@
}
@Override
- public <K, V> Cache<K, V> buildImpl(
- PersistentCacheDef<K, V> in, long limit, CacheBackend backend) {
+ public <K, V> Cache<K, V> buildImpl(PersistentCacheDef<K, V> in, long limit) {
ChronicleMapCacheConfig config =
configFactory.create(
in.configKey(),
fileName(cacheDir, in.name(), in.version()),
in.expireAfterWrite(),
in.refreshAfterWrite());
- return build(in, backend, config, metricMaker);
+ return build(in, config, metricMaker);
}
@SuppressWarnings("unchecked")
@VisibleForTesting
<K, V> Cache<K, V> build(
- PersistentCacheDef<K, V> in,
- CacheBackend backend,
- ChronicleMapCacheConfig config,
- MetricMaker metricMaker) {
+ PersistentCacheDef<K, V> in, ChronicleMapCacheConfig config, MetricMaker metricMaker) {
ChronicleMapCacheDefProxy<K, V> def = new ChronicleMapCacheDefProxy<>(in);
ChronicleMapCacheImpl<K, V> cache;
@@ -120,7 +115,7 @@
LoadingCache<K, TimedValue<V>> mem =
(LoadingCache<K, TimedValue<V>>)
- memCacheFactory.build(def, (CacheLoader<K, V>) memLoader, backend);
+ memCacheFactory.build(def, (CacheLoader<K, V>) memLoader);
cache =
new ChronicleMapCacheImpl<>(
@@ -142,14 +137,14 @@
@Override
public <K, V> LoadingCache<K, V> buildImpl(
- PersistentCacheDef<K, V> in, CacheLoader<K, V> loader, long limit, CacheBackend backend) {
+ PersistentCacheDef<K, V> in, CacheLoader<K, V> loader, long limit) {
ChronicleMapCacheConfig config =
configFactory.create(
in.configKey(),
fileName(cacheDir, in.name(), in.version()),
in.expireAfterWrite(),
in.refreshAfterWrite());
- return build(in, loader, backend, config, metricMaker);
+ return build(in, loader, config, metricMaker);
}
@SuppressWarnings("unchecked")
@@ -157,7 +152,6 @@
public <K, V> LoadingCache<K, V> build(
PersistentCacheDef<K, V> in,
CacheLoader<K, V> loader,
- CacheBackend backend,
ChronicleMapCacheConfig config,
MetricMaker metricMaker) {
ChronicleMapCacheImpl<K, V> cache;
@@ -173,7 +167,7 @@
LoadingCache<K, TimedValue<V>> mem =
(LoadingCache<K, TimedValue<V>>)
- memCacheFactory.build(def, (CacheLoader<K, V>) memLoader, backend);
+ memCacheFactory.build(def, (CacheLoader<K, V>) memLoader);
cache =
new ChronicleMapCacheImpl<>(
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
index c27be99..8966aa5 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapStoreMetrics.java
@@ -30,7 +30,7 @@
ChronicleMapStoreMetrics(String name, MetricMaker metricMaker) {
this.name = name;
- this.sanitizedName = metricMaker.sanitizeMetricName(name);
+ this.sanitizedName = CacheNameSanitizer.sanitize(metricMaker, name);
this.metricMaker = metricMaker;
this.storePutFailures =
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java
index 58a34e1..c93e44e 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/H2MigrationServlet.java
@@ -171,8 +171,8 @@
}
logger.atInfo().log("Migrating H2 caches to Chronicle-Map...");
- logger.atInfo().log("* Size multiplier: %s", sizeMultiplier);
- logger.atInfo().log("* Max Bloat Factor: %s", maxBloatFactor);
+ logger.atInfo().log("* Size multiplier: %d", sizeMultiplier);
+ logger.atInfo().log("* Max Bloat Factor: %d", maxBloatFactor);
Config outputChronicleMapConfig = new Config();
diff --git a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
index 098e8c6..380648c 100644
--- a/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
+++ b/src/main/java/com/googlesource/gerrit/modules/cache/chroniclemap/SerializationMetricsForCache.java
@@ -35,7 +35,7 @@
final Timer0 serializeLatency;
private Metrics(MetricMaker metricMaker, String cacheName) {
- String sanitizedName = metricMaker.sanitizeMetricName(cacheName);
+ String sanitizedName = CacheNameSanitizer.sanitize(metricMaker, cacheName);
deserializeLatency =
metricMaker.newTimer(
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizerTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizerTest.java
new file mode 100644
index 0000000..ec469e4
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/CacheNameSanitizerTest.java
@@ -0,0 +1,49 @@
+// Copyright (C) 2023 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.googlesource.gerrit.modules.cache.chroniclemap;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.googlesource.gerrit.modules.cache.chroniclemap.CacheNameSanitizer.sanitize;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.gerrit.metrics.MetricMaker;
+import org.junit.Test;
+
+public class CacheNameSanitizerTest {
+ @Test
+ public void shouldNotSanitizeTypicalCacheName() {
+ String cacheName = "diff_summary";
+
+ assertThat(sanitize(mock(MetricMaker.class), cacheName)).isEqualTo(cacheName);
+ }
+
+ @Test
+ public void shouldNotSanitizeCacheNameWithHyphens() {
+ String cacheName = "cache_name-with-hyphens";
+
+ assertThat(sanitize(mock(MetricMaker.class), cacheName)).isEqualTo(cacheName);
+ }
+
+ @Test
+ public void shouldFallbackToMetricMakerSanitization() {
+ MetricMaker metricMaker = mock(MetricMaker.class);
+ String sanitizedName = "sanitized";
+ when(metricMaker.sanitizeMetricName(anyString())).thenReturn(sanitizedName);
+
+ assertThat(sanitize(metricMaker, "very+confusing.cache#name")).isEqualTo(sanitizedName);
+ }
+}
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheExtendedIT.java
similarity index 98%
rename from src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
rename to src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheExtendedIT.java
index 4fe1614..6fec793 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheTest.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheExtendedIT.java
@@ -28,7 +28,6 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.metrics.DisabledMetricMaker;
import com.google.gerrit.metrics.MetricMaker;
-import com.google.gerrit.server.cache.CacheBackend;
import com.google.gerrit.server.cache.MemoryCacheFactory;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
import com.google.gerrit.server.cache.serialize.StringCacheSerializer;
@@ -49,7 +48,7 @@
import org.junit.runner.Description;
@UseLocalDisk // Needed to have Gerrit with DropWizardMetricMaker enabled
-public class ChronicleMapCacheTest extends AbstractDaemonTest {
+public class ChronicleMapCacheExtendedIT extends AbstractDaemonTest {
private static final DisabledMetricMaker WITHOUT_METRICS = new DisabledMetricMaker();
@Inject MetricMaker metricMaker;
@Inject MetricRegistry metricRegistry;
@@ -593,7 +592,7 @@
@Test
public void shouldSanitizeUnwantedCharsInMetricNames() throws Exception {
String cacheName = "very+confusing.cache#name";
- String sanitized = "very_confusing_cache_name";
+ String sanitized = "very_0x2B_confusing_0x2E_cache_0x23_name";
String percentageFreeMetricName = "cache/chroniclemap/percentage_free_space_" + sanitized;
String autoResizeMetricName = "cache/chroniclemap/remaining_autoresizes_" + sanitized;
String maxAutoResizeMetricName = "cache/chroniclemap/max_autoresizes_" + sanitized;
@@ -701,11 +700,10 @@
if (withLoader) {
return (ChronicleMapCacheImpl<String, String>)
- cacheFactory.build(
- cacheDef, cacheDef.loader(), CacheBackend.CAFFEINE, config, metricMaker);
+ cacheFactory.build(cacheDef, cacheDef.loader(), config, metricMaker);
}
return (ChronicleMapCacheImpl<String, String>)
- cacheFactory.build(cacheDef, CacheBackend.CAFFEINE, config, metricMaker);
+ cacheFactory.build(cacheDef, config, metricMaker);
}
private ChronicleMapCacheImpl<String, String> newCacheWithLoader(@Nullable String loadedValue) {
diff --git a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheIT.java b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheIT.java
index 389fbb1..9367727 100644
--- a/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheIT.java
+++ b/src/test/java/com/googlesource/gerrit/modules/cache/chroniclemap/ChronicleMapCacheIT.java
@@ -21,7 +21,6 @@
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.accounts.AccountInput;
-import com.google.gerrit.server.cache.CacheBackend;
import com.google.gerrit.server.cache.PersistentCacheFactory;
import com.google.inject.Inject;
import org.junit.Test;
@@ -46,8 +45,7 @@
public void shouldBuildInMemoryCacheWhenDiskLimitIsNegative() {
final int negativeDiskLimit = -1;
final Cache<String, String> cache =
- persistentCacheFactory.build(
- new TestPersistentCacheDef("foo", null, negativeDiskLimit, 0), CacheBackend.CAFFEINE);
+ persistentCacheFactory.build(new TestPersistentCacheDef("foo", null, negativeDiskLimit, 0));
assertThat(cache.getClass().getSimpleName()).isEqualTo("CaffeinatedGuavaCache");
}
@@ -57,8 +55,7 @@
final int positiveDiskLimit = 1024;
assertThat(
persistentCacheFactory.build(
- new TestPersistentCacheDef("foo", null, positiveDiskLimit, ZERO_INMEMORY_CACHE),
- CacheBackend.CAFFEINE))
+ new TestPersistentCacheDef("foo", null, positiveDiskLimit, ZERO_INMEMORY_CACHE)))
.isInstanceOf(ChronicleMapCacheImpl.class);
}
