Merge branch 'stable-2.15' into stable-2.16 * stable-2.15: Allow to configure the number of hook execution workers Change-Id: I1b7d98d9347a5d59c7316a1dc301211ccd967add
diff --git a/BUILD b/BUILD index 4f332ac..cfdc295 100644 --- a/BUILD +++ b/BUILD
@@ -1,4 +1,5 @@ -load("//tools/bzl:plugin.bzl", "gerrit_plugin") +load("//tools/bzl:junit.bzl", "junit_tests") +load("//tools/bzl:plugin.bzl", "PLUGIN_DEPS", "PLUGIN_TEST_DEPS", "gerrit_plugin") gerrit_plugin( name = "hooks", @@ -9,3 +10,13 @@ ], resources = glob(["src/main/resources/**/*"]), ) + +junit_tests( + name = "hooks_tests", + srcs = glob(["src/test/java/**/*.java"]), + tags = ["hooks"], + visibility = ["//visibility:public"], + deps = PLUGIN_TEST_DEPS + PLUGIN_DEPS + [ + ":hooks__plugin", + ], +)
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/CommitReceived.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/CommitReceived.java index b03a77b..74a0817 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/CommitReceived.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/CommitReceived.java
@@ -51,7 +51,7 @@ args.add("--project", projectName); args.add("--refname", refname); args.add("--uploader", receiveEvent.user.getNameEmail()); - args.add("--uploader-username", receiveEvent.user.getUserName()); + args.add("--uploader-username", receiveEvent.user.getUserName().orElse(null)); args.add("--oldrev", old.name()); args.add("--newrev", receiveEvent.commit.name()); args.add("--cmdref", commandRef);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java index 91d3696..87d4291 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookArgs.java
@@ -18,20 +18,22 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; -import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ApprovalInfo; import com.google.gerrit.extensions.common.ChangeInfo; +import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.IdentifiedUser; -import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.config.UrlFormatter; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.inject.Inject; -import com.google.inject.Provider; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Optional; class HookArgs { interface Factory { @@ -39,22 +41,22 @@ } final IdentifiedUser.GenericFactory identifiedUserFactory; - final Provider<String> urlProvider; final HookMetrics metrics; final GitRepositoryManager gitManager; final SitePaths sitePaths; + final DynamicItem<UrlFormatter> urlFormatter; private final List<String> args; @Inject HookArgs( IdentifiedUser.GenericFactory identifiedUserFactory, - @CanonicalWebUrl @Nullable Provider<String> urlProvider, + DynamicItem<UrlFormatter> urlFormatter, HookMetrics metrics, GitRepositoryManager gitManager, SitePaths sitePaths) { this.identifiedUserFactory = identifiedUserFactory; - this.urlProvider = urlProvider; + this.urlFormatter = urlFormatter; this.metrics = metrics; this.gitManager = gitManager; this.sitePaths = sitePaths; @@ -97,9 +99,14 @@ public void addUrl(ChangeInfo change) { args.add("--change-url"); - String url = urlProvider.get(); - if (change != null && url != null) { - args.add(url + change._number); + if (change != null) { + Optional<UrlFormatter> uf = Optional.ofNullable(urlFormatter.get()); + args.add( + uf.flatMap( + f -> + f.getChangeViewUrl( + new Project.NameKey(change.project), new Change.Id(change._number))) + .orElse("")); } else { args.add(""); }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java index 42d4e75..0232f9b 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookExecutor.java
@@ -1,8 +1,10 @@ package com.googlesource.gerrit.plugins.hooks; +import com.google.common.flogger.FluentLogger; import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.logging.LoggingContextAwareExecutorService; import com.google.inject.Inject; import java.lang.Thread.UncaughtExceptionHandler; import java.nio.file.Files; @@ -13,18 +15,13 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import org.eclipse.jgit.lib.Config; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class HookExecutor implements LifecycleListener { - private static final Logger log = LoggerFactory.getLogger(HookExecutor.class); + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private static final UncaughtExceptionHandler LOG_UNCAUGHT_EXCEPTION = - new UncaughtExceptionHandler() { - @Override - public void uncaughtException(Thread t, Throwable e) { - log.error("HookExecutor thread {} threw exception", t.getName(), e); - } - }; + (t, e) -> + logger.atSevere().withCause(e).log("HookExecutor thread %s threw exception", t.getName()); private final ExecutorService threadPool; private final int timeout; @@ -33,11 +30,12 @@ HookExecutor(@GerritServerConfig Config config) { this.timeout = config.getInt("hooks", "syncHookTimeout", 30); this.threadPool = - Executors.newCachedThreadPool( - new ThreadFactoryBuilder() - .setNameFormat("SyncHook-%d") - .setUncaughtExceptionHandler(LOG_UNCAUGHT_EXCEPTION) - .build()); + new LoggingContextAwareExecutorService( + Executors.newCachedThreadPool( + new ThreadFactoryBuilder() + .setNameFormat("SyncHook-%d") + .setUncaughtExceptionHandler(LOG_UNCAUGHT_EXCEPTION) + .build())); } HookResult submit(Path hook, HookArgs args) { @@ -46,7 +44,7 @@ HookResult submit(String projectName, Path hook, HookArgs args) { if (!Files.exists(hook)) { - log.debug("Hook file not found: {}", hook); + logger.atFine().log("Hook file not found: %s", hook); return null; } HookTask.Sync hookTask = new HookTask.Sync(projectName, hook, args); @@ -58,10 +56,10 @@ return task.get(timeout, TimeUnit.SECONDS); } catch (TimeoutException e) { message = "Synchronous hook timed out " + hook; - log.error(message); + logger.atSevere().log(message); } catch (Exception e) { message = "Error running hook " + hook; - log.error(message, e); + logger.atSevere().withCause(e).log(message); } task.cancel(true); hookTask.cancel();
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java index f3d8293..afd189d 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookFactory.java
@@ -16,6 +16,7 @@ import static com.google.common.base.MoreObjects.firstNonNull; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.SitePaths; import com.google.inject.Inject; @@ -23,12 +24,10 @@ import java.nio.file.Path; import java.nio.file.Paths; import org.eclipse.jgit.lib.Config; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; @Singleton public class HookFactory { - private static final Logger log = LoggerFactory.getLogger(HookFactory.class); + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final HookQueue queue; private final HookExecutor syncHookExecutor; @@ -54,13 +53,13 @@ } else { this.hooksPath = sitePaths.hooks_dir; } - log.info("hooks.path: {}", this.hooksPath); + logger.atInfo().log("hooks.path: %s", this.hooksPath); } private Path getHookPath(String configName, String defaultName) { String v = config.getString("hooks", null, configName); Path hookPath = hooksPath.resolve(firstNonNull(v, defaultName)); - log.info("hooks.{} resolved to {}", configName, hookPath); + logger.atInfo().log("hooks.%s resolved to %s", configName, hookPath); return hookPath; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java index 86c457d..50ba975 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookQueue.java
@@ -14,6 +14,7 @@ package com.googlesource.gerrit.plugins.hooks; +import com.google.common.flogger.FluentLogger; import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.WorkQueue; @@ -22,11 +23,9 @@ import java.nio.file.Path; import java.util.concurrent.ScheduledExecutorService; import org.eclipse.jgit.lib.Config; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; class HookQueue implements LifecycleListener { - private static final Logger log = LoggerFactory.getLogger(HookQueue.class); + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final WorkQueue workQueue; private final int executorThreads; @@ -45,7 +44,7 @@ void submit(String projectName, Path hook, HookArgs args) { if (!Files.exists(hook)) { - log.debug("Hook file not found: {}", hook.toAbsolutePath()); + logger.atFine().log("Hook file not found: %s", hook.toAbsolutePath()); return; } queue.submit(new HookTask.Async(projectName, hook, args));
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java index b6654da..8b06a24 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/HookTask.java
@@ -16,6 +16,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.flogger.FluentLogger; import com.google.common.io.ByteStreams; import com.google.gerrit.metrics.Timer1; import com.google.gerrit.reviewdb.client.Project; @@ -29,11 +30,9 @@ import java.util.Map; import java.util.concurrent.Callable; import org.eclipse.jgit.lib.Repository; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; class HookTask { - private static final Logger log = LoggerFactory.getLogger(HookTask.class); + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final Path sitePath; private final String projectName; @@ -113,25 +112,25 @@ } catch (InterruptedException iex) { // InterruptedException - timeout or cancel args.metrics.timeout(name); - log.error("hook[{}] timed out: {}", name, iex.getMessage()); + logger.atSevere().log("hook[%s] timed out: %s", name, iex.getMessage()); } catch (Throwable err) { args.metrics.error(name); - log.error("Error running hook {}", hook.toAbsolutePath(), err); + logger.atSevere().withCause(err).log("Error running hook %s", hook.toAbsolutePath()); } if (result != null) { int exitValue = result.getExitValue(); if (exitValue != 0) { - log.error("hook[{}] exited with error status: {}", name, exitValue); + logger.atSevere().log("hook[%s] exited with error status: %d", name, exitValue); } - if (log.isDebugEnabled()) { + if (logger.atFine().isEnabled()) { try (BufferedReader br = new BufferedReader(new StringReader(result.getOutput()))) { br.lines() .filter(s -> !s.isEmpty()) - .forEach(line -> log.debug("hook[{}] output: {}", name, line)); + .forEach(line -> logger.atFine().log("hook[%s] output: %s", name, line)); } catch (IOException iox) { - log.error("Error writing hook [{}] output", name, iox); + logger.atSevere().withCause(iox).log("Error writing hook [%s] output", name); } } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/RefUpdate.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/RefUpdate.java index 7eedfa5..90a1b93 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/RefUpdate.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/RefUpdate.java
@@ -49,7 +49,7 @@ HookArgs args = hookFactory.createArgs(); args.add("--project", projectName); args.add("--uploader", refEvent.user.getNameEmail()); - args.add("--uploader-username", refEvent.user.getUserName()); + args.add("--uploader-username", refEvent.user.getUserName().orElse(null)); args.add("--oldrev", getObjectId(refEvent.command.getOldId()).getName()); args.add("--newrev", getObjectId(refEvent.command.getNewId()).getName()); args.add("--refname", refEvent.command.getRefName());
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/Submit.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/Submit.java index 8e2f1e0..e383200 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/Submit.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/Submit.java
@@ -51,7 +51,7 @@ args.add("--project", projectName); args.add("--branch", destBranch.get()); args.add("--submitter", caller.getNameEmail()); - args.add("--submitter-username", caller.getUserName()); + args.add("--submitter-username", caller.getUserName().orElse(null)); args.add("--patchset", patchSetId.get()); args.add("--commit", commit.getId().name());
diff --git a/src/test/java/com/googlesource/gerrit/plugins/hooks/HooksIT.java b/src/test/java/com/googlesource/gerrit/plugins/hooks/HooksIT.java new file mode 100644 index 0000000..bcf2d20 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/hooks/HooksIT.java
@@ -0,0 +1,28 @@ +// Copyright (C) 2019 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.plugins.hooks; + +import com.google.gerrit.acceptance.LightweightPluginDaemonTest; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.TestPlugin; +import org.junit.Test; + +@NoHttpd +@TestPlugin(name = "hooks", sysModule = "com.googlesource.gerrit.plugins.hooks.Module") +public class HooksIT extends LightweightPluginDaemonTest { + + @Test + public void doNothing() throws Exception {} +}