Merge branch 'stable-3.7' * stable-3.7: Rename ChronicleMapCacheTest to ChronicleMapCacheExtendedIT Split cache-chroniclemap ITs into a separate build targets Adjust cache-chroniclemap to new metric sanitization scheme Change-Id: If6c600e49ce9d8e08365bc0aa143623a8fd1a033
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;