Merge branch 'stable-3.6' into stable-3.7
* stable-3.6:
Rename ChronicleMapCacheTest to ChronicleMapCacheExtendedIT
Split cache-chroniclemap ITs into a separate build targets
Adjust cache-chroniclemap to new metric sanitization scheme
Change-Id: Ibe52ae606b2d4567a70664fb6a8a574e71dde751
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 c402c87..5ae2874 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/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/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 99%
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 58cc4ec..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
@@ -48,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;
@@ -592,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;
