Merge branch 'stable-3.7' into stable-3.8
* stable-3.7:
Update plugin-manager submodule to ba74d4969
Fix project name's based submetric names collision
Fix GerritServer replica mode when used with @UseLocalDisk
Format bazel files using buildifier
Format sources using gjf
Add jgit-ssh-jsch as licensed under JGit's license
Release-Notes: skip
Change-Id: Id2c598910edd3663fc28d9a535a94fac6f7a51b2
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index 6c9dfef..93e0eb4 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -375,7 +375,14 @@
* `cache_used_per_repository` : Bytes of memory retained per repository for the
top N repositories having most data in the cache. The number N of reported
repositories is limited to 1000.
-** `repository_name`: The name of the repository.
+** `repository_name`: The name of the repository. Note that it is a subject of
+ sanitization in order to avoid collision between repository names. Rules
+ are:
+*** any character outside `[a-zA-Z0-9_-]+([a-zA-Z0-9_-]+)*` pattern is replaced
+ with `\_0x[HEX CODE]_` (code is capitalized) string
+*** for instance `repo/name` is sanitized to `repo_0x2F_name`
+*** if repository name contains the replacement prefix (`_0x`) it is prefixed
+ with another `_0x` e.g. `repo_0x2F_name` becomes `repo_0x_0x2F_name`
=== Git
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index 36bc3c4..e812cb7 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -452,6 +452,12 @@
bind(TestTicker.class).toInstance(testTicker);
}
});
+ daemon.setEnableHttpd(desc.httpd());
+ // Assure that SSHD is enabled if HTTPD is not required, otherwise the Gerrit server would not
+ // even start.
+ daemon.setEnableSshd(!desc.httpd() || desc.useSsh());
+ daemon.setReplica(
+ ReplicaUtil.isReplica(baseConfig) || ReplicaUtil.isReplica(desc.buildConfig(baseConfig)));
if (desc.memory()) {
checkArgument(additionalArgs.length == 0, "cannot pass args to in-memory server");
@@ -468,7 +474,6 @@
@Nullable InMemoryRepositoryManager inMemoryRepoManager)
throws Exception {
Config cfg = desc.buildConfig(baseConfig);
- daemon.setReplica(ReplicaUtil.isReplica(baseConfig) || ReplicaUtil.isReplica(cfg));
mergeTestConfig(cfg);
// Set the log4j configuration to an invalid one to prevent system logs
// from getting configured and creating log files.
@@ -745,4 +750,8 @@
public String toString() {
return MoreObjects.toStringHelper(this).addValue(desc).toString();
}
+
+ public boolean isReplica() {
+ return daemon.isReplica();
+ }
}
diff --git a/java/com/google/gerrit/metrics/Field.java b/java/com/google/gerrit/metrics/Field.java
index 5508819..01b89cf 100644
--- a/java/com/google/gerrit/metrics/Field.java
+++ b/java/com/google/gerrit/metrics/Field.java
@@ -21,6 +21,8 @@
import java.util.Optional;
import java.util.function.BiConsumer;
import java.util.function.Function;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
/**
* Describes a bucketing field used by a metric.
@@ -102,6 +104,21 @@
.metadataMapper(metadataMapper);
}
+ /**
+ * A dedicated field to be used with metrics based on {@link Metadata#projectName()}. It was
+ * introduced to sanitize the project name to avoid sub-metric name's collision.
+ *
+ * @param fieldName name of the field that contains a project name as value
+ * @return builder for the project name field
+ */
+ public static Field.Builder<String> ofProjectName(String fieldName) {
+ return new AutoValue_Field.Builder<String>()
+ .valueType(String.class)
+ .formatter(Field::sanitizeProjectName)
+ .name(fieldName)
+ .metadataMapper(Metadata.Builder::projectName);
+ }
+
/** Returns name of this field within the metric. */
public abstract String name();
@@ -137,4 +154,33 @@
return field;
}
}
+
+ private static final Pattern SUBMETRIC_NAME_PATTERN =
+ Pattern.compile("[a-zA-Z0-9_-]+([a-zA-Z0-9_-]+)*");
+ private static final Pattern INVALID_CHAR_PATTERN = Pattern.compile("[^\\w-]");
+ private static final String REPLACEMENT_PREFIX = "_0x";
+
+ private static String sanitizeProjectName(String projectName) {
+ if (SUBMETRIC_NAME_PATTERN.matcher(projectName).matches()
+ && !projectName.contains(REPLACEMENT_PREFIX)) {
+ return projectName;
+ }
+
+ String replacmentPrefixSanitizedName =
+ projectName.replaceAll(REPLACEMENT_PREFIX, REPLACEMENT_PREFIX + REPLACEMENT_PREFIX);
+ StringBuilder sanitizedName = new StringBuilder();
+ for (int i = 0; i < replacmentPrefixSanitizedName.length(); i++) {
+ Character c = replacmentPrefixSanitizedName.charAt(i);
+ Matcher matcher = INVALID_CHAR_PATTERN.matcher(c.toString());
+ if (matcher.matches()) {
+ sanitizedName.append(REPLACEMENT_PREFIX);
+ sanitizedName.append(Integer.toHexString(c).toUpperCase());
+ sanitizedName.append('_');
+ } else {
+ sanitizedName.append(c);
+ }
+ }
+
+ return sanitizedName.toString();
+ }
}
diff --git a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java
index d64bd19..8771448 100644
--- a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java
+++ b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java
@@ -20,7 +20,6 @@
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.Field;
import com.google.gerrit.metrics.MetricMaker;
-import com.google.gerrit.server.logging.Metadata;
import java.util.Map;
import org.eclipse.jgit.storage.file.WindowCacheStats;
@@ -184,7 +183,7 @@
+ "having most data in the cache.")
.setGauge()
.setUnit("byte"),
- Field.ofString("repository_name", Metadata.Builder::projectName)
+ Field.ofProjectName("repository_name")
.description("The name of the repository.")
.build());
metrics.newTrigger(
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 744f91b..7474709 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -250,6 +250,10 @@
this.replica = replica;
}
+ public boolean isReplica() {
+ return replica;
+ }
+
@VisibleForTesting
public Injector getHttpdInjector() {
return httpdInjector;
@@ -375,6 +379,7 @@
}
cfgInjector = createCfgInjector();
config = cfgInjector.getInstance(Key.get(Config.class, GerritServerConfig.class));
+ config.setBoolean("container", null, "replica", replica);
indexType = IndexModule.getIndexType(cfgInjector);
sysInjector = createSysInjector();
sysInjector.getInstance(PluginGuiceEnvironment.class).setDbCfgInjector(dbInjector, cfgInjector);
diff --git a/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java b/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java
index bd07f7d..213cd1c 100644
--- a/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java
+++ b/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.config;
+import com.google.gerrit.server.util.ReplicaUtil;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -26,21 +27,15 @@
*/
@Singleton
public final class GerritIsReplicaProvider implements Provider<Boolean> {
- public static final String CONFIG_SECTION = "container";
- public static final String REPLICA_KEY = "replica";
- public static final String DEPRECATED_REPLICA_KEY = "slave";
-
- public final boolean isReplica;
+ private final Config config;
@Inject
public GerritIsReplicaProvider(@GerritServerConfig Config config) {
- this.isReplica =
- config.getBoolean(CONFIG_SECTION, REPLICA_KEY, false)
- || config.getBoolean(CONFIG_SECTION, DEPRECATED_REPLICA_KEY, false);
+ this.config = config;
}
@Override
public Boolean get() {
- return isReplica;
+ return ReplicaUtil.isReplica(config);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/server/config/GerritIsReplicaIT.java b/javatests/com/google/gerrit/acceptance/server/config/GerritIsReplicaIT.java
index d01a81d..b8ec57b 100644
--- a/javatests/com/google/gerrit/acceptance/server/config/GerritIsReplicaIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/config/GerritIsReplicaIT.java
@@ -18,6 +18,8 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.Sandboxed;
+import com.google.gerrit.acceptance.UseLocalDisk;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.server.config.GerritIsReplicaProvider;
import com.google.gerrit.testing.ConfigSuite;
import com.google.inject.Inject;
@@ -35,6 +37,14 @@
@Test
public void isNotReplica() {
assertThat(isReplicaProvider.get()).isFalse();
+ assertThat(server.isReplica()).isFalse();
+ }
+
+ @Test
+ @UseLocalDisk
+ public void isNotReplicaWithLocalDisk() {
+ assertThat(isReplicaProvider.get()).isFalse();
+ assertThat(server.isReplica()).isFalse();
}
@Test
@@ -42,5 +52,30 @@
public void isReplica() throws Exception {
restartAsSlave();
assertThat(isReplicaProvider.get()).isTrue();
+ assertThat(server.isReplica()).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "container.replica", value = "true")
+ public void isReplicaFromGerritConfigAnnotation() throws Exception {
+ assertThat(isReplicaProvider.get()).isTrue();
+ assertThat(server.isReplica()).isTrue();
+ }
+
+ @Test
+ @UseLocalDisk
+ @GerritConfig(name = "container.replica", value = "true")
+ public void isReplicaWithLocalDisk() throws Exception {
+ assertThat(isReplicaProvider.get()).isTrue();
+ assertThat(server.isReplica()).isTrue();
+ }
+
+ @Test
+ @Sandboxed
+ @UseLocalDisk
+ public void isReplicaWithLocalDiskAfterRestart() throws Exception {
+ restartAsSlave();
+ assertThat(isReplicaProvider.get()).isTrue();
+ assertThat(server.isReplica()).isTrue();
}
}
diff --git a/javatests/com/google/gerrit/metrics/BUILD b/javatests/com/google/gerrit/metrics/BUILD
new file mode 100644
index 0000000..531280e
--- /dev/null
+++ b/javatests/com/google/gerrit/metrics/BUILD
@@ -0,0 +1,13 @@
+load("//tools/bzl:junit.bzl", "junit_tests")
+
+junit_tests(
+ name = "field_tests",
+ size = "small",
+ srcs = glob(["*.java"]),
+ tags = ["metrics"],
+ deps = [
+ "//java/com/google/gerrit/metrics",
+ "//java/com/google/gerrit/testing:gerrit-test-util",
+ "//lib/truth",
+ ],
+)
diff --git a/javatests/com/google/gerrit/metrics/FieldSanitizeProjectNameTest.java b/javatests/com/google/gerrit/metrics/FieldSanitizeProjectNameTest.java
new file mode 100644
index 0000000..f12b1b3
--- /dev/null
+++ b/javatests/com/google/gerrit/metrics/FieldSanitizeProjectNameTest.java
@@ -0,0 +1,54 @@
+// 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.google.gerrit.metrics;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import java.util.Arrays;
+import java.util.Collection;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class FieldSanitizeProjectNameTest {
+ @Parameterized.Parameters
+ public static Collection<Object[]> testData() {
+ return Arrays.asList(
+ new Object[][] {
+ {"repoName", "repoName"},
+ {"repo_name", "repo_name"},
+ {"repo-name", "repo-name"},
+ {"repo/name", "repo_0x2F_name"},
+ {"repo+name", "repo_0x2B_name"},
+ {"repo_0x2F_name", "repo_0x_0x2F_name"},
+ });
+ }
+
+ private final String input;
+ private final String expected;
+
+ public FieldSanitizeProjectNameTest(String input, String expected) {
+ this.input = input;
+ this.expected = expected;
+ }
+
+ @Test
+ public void shouldSanitizeProjectName() {
+ Field<String> projectNameField = Field.ofProjectName("test_name").build();
+ String result = projectNameField.formatter().apply(input);
+ assertThat(result).isEqualTo(expected);
+ }
+}
diff --git a/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java b/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
index b1cd8fb..512a1b1 100644
--- a/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
+++ b/javatests/com/google/gerrit/server/logging/PerformanceLogContextTest.java
@@ -171,7 +171,7 @@
new Description("Latency metric for testing"),
Field.ofInteger("account", Metadata.Builder::accountId).build(),
Field.ofInteger("change", Metadata.Builder::changeId).build(),
- Field.ofString("project", Metadata.Builder::projectName).build());
+ Field.ofProjectName("project").build());
timer3.start(1000000, 123, "foo/bar").close();
assertThat(LoggingContext.getInstance().getPerformanceLogRecords()).hasSize(4);
@@ -206,14 +206,14 @@
metricMaker.newTimer(
"test1/latency",
new Description("Latency metric for testing"),
- Field.ofString("project", Metadata.Builder::projectName).build());
+ Field.ofProjectName("project").build());
timer1.start(null).close();
Timer2<String, String> timer2 =
metricMaker.newTimer(
"test2/latency",
new Description("Latency metric for testing"),
- Field.ofString("project", Metadata.Builder::projectName).build(),
+ Field.ofProjectName("project").build(),
Field.ofString("branch", Metadata.Builder::branchName).build());
timer2.start(null, null).close();
@@ -221,7 +221,7 @@
metricMaker.newTimer(
"test3/latency",
new Description("Latency metric for testing"),
- Field.ofString("project", Metadata.Builder::projectName).build(),
+ Field.ofProjectName("project").build(),
Field.ofString("branch", Metadata.Builder::branchName).build(),
Field.ofString("revision", Metadata.Builder::revision).build());
timer3.start(null, null, null).close();
@@ -270,7 +270,7 @@
new Description("Latency metric for testing"),
Field.ofInteger("account", Metadata.Builder::accountId).build(),
Field.ofInteger("change", Metadata.Builder::changeId).build(),
- Field.ofString("project", Metadata.Builder::projectName).build());
+ Field.ofProjectName("project").build());
timer3.start(1000000, 123, "value3").close();
assertThat(LoggingContext.getInstance().isPerformanceLogging()).isFalse();
@@ -317,7 +317,7 @@
new Description("Latency metric for testing"),
Field.ofInteger("account", Metadata.Builder::accountId).build(),
Field.ofInteger("change", Metadata.Builder::changeId).build(),
- Field.ofString("project", Metadata.Builder::projectName).build());
+ Field.ofProjectName("project").build());
timer3.start(1000000, 123, "foo/bar").close();
assertThat(LoggingContext.getInstance().getPerformanceLogRecords()).isEmpty();
diff --git a/plugins/plugin-manager b/plugins/plugin-manager
index dbd6820..ba74d49 160000
--- a/plugins/plugin-manager
+++ b/plugins/plugin-manager
@@ -1 +1 @@
-Subproject commit dbd68200d867513e2c0449798476e275aaf08cfd
+Subproject commit ba74d4969462c2592bcf97868dd76c33041d47b2
