From 773777e61335a65c8a73969a0dc9503ab263d33f Mon Sep 17 00:00:00 2001 From: Thomas Draebing Date: Mon, 27 Jul 2020 12:44:10 +0200 Subject: [PATCH 1/5] Respect log.textLogging and log.jsonLogging using --console-log The gerrit.war daemon provides a --console-log flag to send the error logs to stderr. However, when using this flag, no logs were written to the error_log or error_log.json files, even if the corresponding option, log.textLogging or log.jsonLogging, was set in the gerrit.config file. On the one hand, the configuration should be respected and on the other hand, there are usecases, where both output channels are useful. Mainly, if Gerrit is run in a docker container, it is useful to be able to use 'docker logs' to view the current logs and to have the logs persisted at the same time. While one can tail the log-file to do so, Gerrit is then not the main process of the container and will thus not be gracefully shutdown, when stopping the container. Use the --console-log option together with {text|json}Logging to log to stderr and the error_log file without the need to tail the file to stdout. Note, that this will as a side effect change the default behaviour of the --console-log flag. Since the log.textLogging option is true by default, using the --console-log flag will now by default log to stderr and the error_log file. This can be prevented by setting log.textLogging to 'false'. Bug: Issue 13184 Change-Id: I5c3dfa531a6ccd397d3ceb60defc6e83943e95f1 --- Documentation/pgm-daemon.txt | 5 ++-- java/com/google/gerrit/pgm/Daemon.java | 4 +-- .../google/gerrit/pgm/util/ErrorLogFile.java | 25 +++++++++++++------ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/Documentation/pgm-daemon.txt b/Documentation/pgm-daemon.txt index ad07cfac1f..25ca4dd706 100644 --- a/Documentation/pgm-daemon.txt +++ b/Documentation/pgm-daemon.txt @@ -49,8 +49,9 @@ per the local copy of link:config-gerrit.html[gerrit.config] located under This option automatically implies '--enable-sshd'. --console-log:: - Send log messages to the console, instead of to the standard - log file '$site_path/logs/error_log'. + Send log messages to the console. Log files will still be written to + the error log file, if log.textLogging and/or log.jsonLogging is set to + 'true'. --headless:: Don't start the default Gerrit UI. May be useful when Gerrit is diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index 65534770fe..d2371e896e 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -331,9 +331,7 @@ public class Daemon extends SiteProgram { sysInjector.getInstance(PluginGuiceEnvironment.class).setDbCfgInjector(dbInjector, cfgInjector); manager.add(dbInjector, cfgInjector, sysInjector); - if (!consoleLog) { - manager.add(ErrorLogFile.start(getSitePath(), config)); - } + manager.add(ErrorLogFile.start(getSitePath(), config, consoleLog)); sshd &= !sshdOff(); if (sshd) { diff --git a/java/com/google/gerrit/pgm/util/ErrorLogFile.java b/java/com/google/gerrit/pgm/util/ErrorLogFile.java index 227719ab79..8eae82ac92 100644 --- a/java/com/google/gerrit/pgm/util/ErrorLogFile.java +++ b/java/com/google/gerrit/pgm/util/ErrorLogFile.java @@ -49,11 +49,12 @@ public class ErrorLogFile { root.addAppender(dst); } - public static LifecycleListener start(Path sitePath, Config config) throws IOException { + public static LifecycleListener start(Path sitePath, Config config, boolean consoleLog) + throws IOException { Path logdir = FileUtil.mkdirsOrDie(new SitePaths(sitePath).logs_dir, "Cannot create log directory"); if (SystemLog.shouldConfigure()) { - initLogSystem(logdir, config); + initLogSystem(logdir, config, consoleLog); } return new LifecycleListener() { @@ -67,18 +68,28 @@ public class ErrorLogFile { }; } - private static void initLogSystem(Path logdir, Config config) { + private static void initLogSystem(Path logdir, Config config, boolean consoleLog) { Logger root = LogManager.getRootLogger(); root.removeAllAppenders(); + PatternLayout errorLogLayout = new PatternLayout("[%d] [%t] %-5p %c %x: %m%n"); + + if (consoleLog) { + ConsoleAppender dst = new ConsoleAppender(); + dst.setLayout(errorLogLayout); + dst.setTarget("System.err"); + dst.setThreshold(Level.INFO); + dst.activateOptions(); + + root.addAppender(dst); + } + boolean json = config.getBoolean("log", "jsonLogging", false); - boolean text = config.getBoolean("log", "textLogging", true) || !json; + boolean text = config.getBoolean("log", "textLogging", true) || !(json || consoleLog); boolean rotate = config.getBoolean("log", "rotate", true); if (text) { - root.addAppender( - SystemLog.createAppender( - logdir, LOG_NAME, new PatternLayout("[%d] [%t] %-5p %c %x: %m%n"), rotate)); + root.addAppender(SystemLog.createAppender(logdir, LOG_NAME, errorLogLayout, rotate)); } if (json) { From 40def8b495ca7d67dd9c982e8f975f50168f435c Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Tue, 4 Aug 2020 16:48:44 -0400 Subject: [PATCH 2/5] Document possibility to resume reviews with meetings In the context of the design doc review process, mention the possibility of resuming stalled reviews through meeting discussions. This may be used or referred to also in other review contexts. Change-Id: I45a1b5deb0311d049a9deffbf8b206c5d0524bc3 --- Documentation/dev-design-docs.txt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Documentation/dev-design-docs.txt b/Documentation/dev-design-docs.txt index be80c94686..a339da396a 100644 --- a/Documentation/dev-design-docs.txt +++ b/Documentation/dev-design-docs.txt @@ -127,6 +127,15 @@ link:dev-processes.html#steering-committee[engineering steering committee] within 14 calendar days whether the proposed feature is in scope of the project and if it can be accepted. +[[meetings]] +=== Meeting discussions + +If the Gerrit review doesn't start efficiently enough, stalls, gets off-track +too much or becomes overly complex, one can use a meeting to refocus it. From +that review thread, the organizer can volunteer oneself, or be proposed (even +requested) by a reviewer. link:https://www.gerritcodereview.com/members.html#community-managers[ +Community managers] may help facilitate that if ultimately necessary. + [[watch-designs]] == How to get notified for new design docs? From 7702e5f28723b5e05a41bf6f6dcee427af3811b6 Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Mon, 3 Aug 2020 17:54:38 -0400 Subject: [PATCH 3/5] Upgrade jackson-core to 2.11.2 Change-Id: Ibfe690fbbee3f79b2ac012326dfeb6dc9b4de886 --- tools/nongoogle.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl index 0313116497..9c664310ad 100644 --- a/tools/nongoogle.bzl +++ b/tools/nongoogle.bzl @@ -100,8 +100,8 @@ def declare_nongoogle_deps(): maven_jar( name = "jackson-core", - artifact = "com.fasterxml.jackson.core:jackson-core:2.11.1", - sha1 = "8b02908d53183fdf9758e7e20f2fdee87613a962", + artifact = "com.fasterxml.jackson.core:jackson-core:2.11.2", + sha1 = "bc022ab0f0c83c07f9c52c5ab9a6a4932b15cc35", ) # Google internal dependencies: these are developed at Google, so there is From 23a99d90135ae54c64938cedb0279f11e453ed9d Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Tue, 25 Aug 2020 16:10:26 -0400 Subject: [PATCH 4/5] Upgrade metrics-core to 4.1.12.1 This upgrade has these dependency updates [2] and bug fixes [1-3]. It should be no negative (only wanted) impact. It is a maintenance upgrade. [1] https://github.com/dropwizard/metrics/releases/tag/v4.1.11 [2] https://github.com/dropwizard/metrics/releases/tag/v4.1.12 [3] https://github.com/dropwizard/metrics/releases/tag/v4.1.12.1 Change-Id: I9647286c96a1a7c6c92d94387ff5909ff154ff7c --- tools/nongoogle.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl index bab1bfa3e4..f0aadbd794 100644 --- a/tools/nongoogle.bzl +++ b/tools/nongoogle.bzl @@ -23,8 +23,8 @@ def declare_nongoogle_deps(): maven_jar( name = "dropwizard-core", - artifact = "io.dropwizard.metrics:metrics-core:4.1.11", - sha1 = "7f05969f40bf7296eac0dbb36c78ada28bf975f6", + artifact = "io.dropwizard.metrics:metrics-core:4.1.12.1", + sha1 = "cb2f351bf4463751201f43bb99865235d5ba07ca", ) SSHD_VERS = "2.4.0" From c862d29ece5d120badb63d87ea85939ee32ad5dc Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 28 Aug 2020 22:26:26 +0100 Subject: [PATCH 5/5] Preserve instanceId on event when already set Plugins can inject external events into Gerrit also coming from other remote instances. Preserve the instanceId contained in the forwarded event if set and do not overwrite with the local Gerrit one. Only events that are locally generated in Gerrit can set the intanceId but not the forwarded ones. Failing to preserve the original instanceId of the forwarded events may cause events duplication, like the use-case of the high-availability plugin not recognising a remote event because it is re-stamped with the local instanceId. Bug: Issue 13307 Change-Id: I324a48a6f2ffca59bad059e18550a8dc44224f34 --- .../gerrit/server/events/EventBroker.java | 15 +++--- .../config/InstanceIdFromPluginIT.java | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/java/com/google/gerrit/server/events/EventBroker.java b/java/com/google/gerrit/server/events/EventBroker.java index 728dd01e21..0fcb64e1ba 100644 --- a/java/com/google/gerrit/server/events/EventBroker.java +++ b/java/com/google/gerrit/server/events/EventBroker.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.events; +import com.google.common.base.Strings; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.BranchNameKey; @@ -111,7 +112,7 @@ public class EventBroker implements EventDispatcher { } protected void fireEvent(Change change, ChangeEvent event) throws PermissionBackendException { - setInstanceId(event); + setInstanceIdWhenEmpty(event); for (PluginSetEntryContext c : listeners) { CurrentUser user = c.call(UserScopedEventListener::getUser); if (isVisibleTo(change, user)) { @@ -122,7 +123,7 @@ public class EventBroker implements EventDispatcher { } protected void fireEvent(Project.NameKey project, ProjectEvent event) { - setInstanceId(event); + setInstanceIdWhenEmpty(event); for (PluginSetEntryContext c : listeners) { CurrentUser user = c.call(UserScopedEventListener::getUser); @@ -135,7 +136,7 @@ public class EventBroker implements EventDispatcher { protected void fireEvent(BranchNameKey branchName, RefEvent event) throws PermissionBackendException { - setInstanceId(event); + setInstanceIdWhenEmpty(event); for (PluginSetEntryContext c : listeners) { CurrentUser user = c.call(UserScopedEventListener::getUser); if (isVisibleTo(branchName, user)) { @@ -146,7 +147,7 @@ public class EventBroker implements EventDispatcher { } protected void fireEvent(Event event) throws PermissionBackendException { - setInstanceId(event); + setInstanceIdWhenEmpty(event); for (PluginSetEntryContext c : listeners) { CurrentUser user = c.call(UserScopedEventListener::getUser); if (isVisibleTo(event, user)) { @@ -156,8 +157,10 @@ public class EventBroker implements EventDispatcher { fireEventForUnrestrictedListeners(event); } - protected void setInstanceId(Event event) { - event.instanceId = gerritInstanceId; + protected void setInstanceIdWhenEmpty(Event event) { + if (Strings.isNullOrEmpty(event.instanceId)) { + event.instanceId = gerritInstanceId; + } } protected boolean isVisibleTo(Project.NameKey project, CurrentUser user) { diff --git a/javatests/com/google/gerrit/acceptance/config/InstanceIdFromPluginIT.java b/javatests/com/google/gerrit/acceptance/config/InstanceIdFromPluginIT.java index 0956de45c3..ac10e96a29 100644 --- a/javatests/com/google/gerrit/acceptance/config/InstanceIdFromPluginIT.java +++ b/javatests/com/google/gerrit/acceptance/config/InstanceIdFromPluginIT.java @@ -19,10 +19,19 @@ import static com.google.common.truth.Truth.assertThat; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.common.Nullable; +import com.google.gerrit.extensions.registration.DynamicItem; +import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.server.config.GerritInstanceId; +import com.google.gerrit.server.events.Event; +import com.google.gerrit.server.events.EventDispatcher; +import com.google.gerrit.server.events.EventListener; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.inject.AbstractModule; import com.google.inject.Inject; +import com.google.inject.Key; import com.google.inject.Scopes; +import java.util.ArrayList; +import java.util.List; import org.junit.Test; @TestPlugin( @@ -35,6 +44,8 @@ public class InstanceIdFromPluginIT extends LightweightPluginDaemonTest { @Override protected void configure() { bind(InstanceIdLoader.class).in(Scopes.SINGLETON); + bind(TestEventListener.class).in(Scopes.SINGLETON); + DynamicSet.bind(binder(), EventListener.class).to(TestEventListener.class); } } @@ -47,6 +58,27 @@ public class InstanceIdFromPluginIT extends LightweightPluginDaemonTest { } } + public static class TestEventListener implements EventListener { + private final List events = new ArrayList<>(); + + @Override + public void onEvent(Event event) { + events.add(event); + } + + public List getEvents() { + return events; + } + } + + public static class TestEvent extends Event { + + protected TestEvent(String instanceId) { + super("test"); + this.instanceId = instanceId; + } + } + @Test @GerritConfig(name = "gerrit.instanceId", value = "testInstanceId") public void shouldReturnInstanceIdWhenDefined() { @@ -58,6 +90,21 @@ public class InstanceIdFromPluginIT extends LightweightPluginDaemonTest { assertThat(getInstanceIdLoader().gerritInstanceId).isNull(); } + @Test + @GerritConfig(name = "gerrit.instanceId", value = "testInstanceId") + public void shouldPreserveEventInstanceIdWhenDefined() throws PermissionBackendException { + EventDispatcher dispatcher = + plugin.getSysInjector().getInstance(new Key>() {}).get(); + String eventInstanceId = "eventInstanceId"; + TestEventListener eventListener = plugin.getSysInjector().getInstance(TestEventListener.class); + TestEvent testEvent = new TestEvent(eventInstanceId); + + dispatcher.postEvent(testEvent); + List receivedEvents = eventListener.getEvents(); + assertThat(receivedEvents).hasSize(1); + assertThat(receivedEvents.get(0).instanceId).isEqualTo(eventInstanceId); + } + private InstanceIdLoader getInstanceIdLoader() { return plugin.getSysInjector().getInstance(InstanceIdLoader.class); }