From 8db58c8fc2002ca1e7373cf6aa32da2626d3d8bf Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Sun, 14 Jun 2020 10:48:22 +0200 Subject: [PATCH 01/11] Report the index state after re-indexing If indexing goes wrong, the failure is typically logged. But we do not explicitly say when re-indexing gave a ready index. One has to resort to double-checking `index/gerrit_index.config` the index state after every re-index. So, we now explicitly report the index state after re-indexing. Change-Id: Idbf8c728a33e2753ee7528dc76f6095f407d3a05 --- java/com/google/gerrit/pgm/Reindex.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/java/com/google/gerrit/pgm/Reindex.java b/java/com/google/gerrit/pgm/Reindex.java index 52d3bd387b..85e860966f 100644 --- a/java/com/google/gerrit/pgm/Reindex.java +++ b/java/com/google/gerrit/pgm/Reindex.java @@ -208,6 +208,9 @@ public class Reindex extends SiteProgram { if (result.success()) { index.markReady(true); } + System.out.format( + "Index %s in version %d is %sready\n", + def.getName(), index.getSchema().getVersion(), result.success() ? "" : "NOT "); return result.success(); } } From 357e2bdea68585f7b3edb71be1b50dc795da705d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 12 Jun 2020 12:06:07 +0200 Subject: [PATCH 02/11] Honor project watches also for changes created via cherry-pick When a change was cherry-picked and a project watch expression from a user matched the newly created change a notification email (because of the project watch) was often not sent. To the contrary, if a new change was created by the create-change workflow then notifications for project watches were honored and notification emails sent. Change the default for the CherryPickInput.notify to ALL, the same like in the CreateChangeInput. Bug: Issue 12918 Change-Id: I52167246940ff68be66625faab3ac3b98280ca33 --- Documentation/rest-api-changes.txt | 2 +- .../google/gerrit/extensions/api/changes/CherryPickInput.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 3bbb6427ab..67b036352f 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -5965,7 +5965,7 @@ Number of the parent relative to which the cherry-pick should be considered. Notify handling that defines to whom email notifications should be sent after the cherry-pick. + Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. + -If not set, the default is `NONE`. +If not set, the default is `ALL`. |`notify_details` |optional| Additional information about whom to notify about the update as a map of recipient type to link:#notify-info[NotifyInfo] entity. diff --git a/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java b/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java index 5ac67e7def..0aace7b02c 100644 --- a/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java +++ b/java/com/google/gerrit/extensions/api/changes/CherryPickInput.java @@ -24,7 +24,7 @@ public class CherryPickInput { public String base; public Integer parent; - public NotifyHandling notify = NotifyHandling.NONE; + public NotifyHandling notify = NotifyHandling.ALL; public Map notifyDetails; public boolean keepReviewers; From 11770a5658ec7d34679f68f6b0bf3b38b94a10cf Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Sun, 14 Jun 2020 11:57:33 +0200 Subject: [PATCH 03/11] Avoid closing System.out after All-Users GC in NoteDB migration By using try-with-resource wrappers around System.out for the garbage collection of the `All-Users` repo, System.out got closed when the wrappers got closed after the garbage collection. Due to the closed System.out, the final status messages of the migration got swallowed and did not make it to the screen. We no longer close the wrappers to keep System.out open. Bug: Issue 12935 Change-Id: I549a027f4ef7bbd863f08532b849e4c43b3d50e1 --- java/com/google/gerrit/pgm/MigrateToNoteDb.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/pgm/MigrateToNoteDb.java b/java/com/google/gerrit/pgm/MigrateToNoteDb.java index 09422b2193..2d375f391b 100644 --- a/java/com/google/gerrit/pgm/MigrateToNoteDb.java +++ b/java/com/google/gerrit/pgm/MigrateToNoteDb.java @@ -149,9 +149,11 @@ public class MigrateToNoteDb extends SiteProgram { migrator.migrate(); } } - try (PrintWriter w = new PrintWriter(new OutputStreamWriter(System.out, UTF_8), true)) { - gcAllUsers.run(w); - } + + PrintWriter w = new PrintWriter(new OutputStreamWriter(System.out, UTF_8), true); + gcAllUsers.run(w); + // No closing of the PrintWriter here, as it would cascade down and eventually close + // System.out and thereby swallow further output. } finally { stop(); } From c5bb255e5c87aa5b3793e7a8ccda8406d5854a11 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Sun, 14 Jun 2020 12:25:30 +0200 Subject: [PATCH 04/11] Allow to re-index in verbose mode during NoteDB migration This makes it easier to see which changes just got indexed and correlate performance graphs with what is going on. Change-Id: Ic3131c997102bfd476326e84dba8cc7206592bbc --- .../google/gerrit/pgm/MigrateToNoteDb.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/java/com/google/gerrit/pgm/MigrateToNoteDb.java b/java/com/google/gerrit/pgm/MigrateToNoteDb.java index 2d375f391b..8d66bada2a 100644 --- a/java/com/google/gerrit/pgm/MigrateToNoteDb.java +++ b/java/com/google/gerrit/pgm/MigrateToNoteDb.java @@ -101,6 +101,9 @@ public class MigrateToNoteDb extends SiteProgram { handler = ExplicitBooleanOptionHandler.class) private Boolean reindex; + @Option(name = "--verbose", usage = "Output more detailed information when reindexing") + private boolean verbose; + private Injector dbInjector; private Injector sysInjector; private LifecycleManager dbManager; @@ -164,14 +167,18 @@ public class MigrateToNoteDb extends SiteProgram { } // Reindex all indices, to save the user from having to run yet another program by hand while // their server is offline. - List reindexArgs = - ImmutableList.of( - "--site-path", - getSitePath().toString(), - "--threads", - Integer.toString(threads), - "--index", - ChangeSchemaDefinitions.NAME); + ImmutableList.Builder reindexArgsBuilder = ImmutableList.builder(); + reindexArgsBuilder.add( + "--site-path", + getSitePath().toString(), + "--threads", + Integer.toString(threads), + "--index", + ChangeSchemaDefinitions.NAME); + if (verbose) { + reindexArgsBuilder.add("--verbose"); + } + List reindexArgs = reindexArgsBuilder.build(); System.out.println("Migration complete, reindexing changes with:"); System.out.println(" reindex " + reindexArgs.stream().collect(joining(" "))); Reindex reindexPgm = new Reindex(); From 27b987ba6101f338dcc619525752562fc0532990 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Sun, 14 Jun 2020 14:52:25 +0200 Subject: [PATCH 05/11] Auto-flush SiteIndexer's PrintWriters As the PrintWriters' auto-flush was not turned on, the last few messages of a NoteDB migration still stuck in buffers when the program stopped and they never made it to the screen. To avoid swallowing (or delaying) output, we turn on auto-flushing. Change-Id: Ib9f74c645a16ebe40717acff9e98c1275bc52518 --- java/com/google/gerrit/index/SiteIndexer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/com/google/gerrit/index/SiteIndexer.java b/java/com/google/gerrit/index/SiteIndexer.java index c3ab8a42e5..f209f2437e 100644 --- a/java/com/google/gerrit/index/SiteIndexer.java +++ b/java/com/google/gerrit/index/SiteIndexer.java @@ -89,7 +89,7 @@ public abstract class SiteIndexer> { } protected PrintWriter newPrintWriter(OutputStream out) { - return new PrintWriter(new OutputStreamWriter(out, UTF_8)); + return new PrintWriter(new OutputStreamWriter(out, UTF_8), true); } private static class ErrorListener implements Runnable { From 3592b2b4f58c3186d7c93034adff4c12a55ecca3 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Sun, 14 Jun 2020 15:27:10 +0200 Subject: [PATCH 06/11] Add project to output when reindexing changes in verbose mode When reindexing changes on multiple threads in parallel, it may be that at first all threads are busy. But near the end most are idling and only a handful are doing work. By printing the projects for each change, we help to expose which projects might benefit from optimization like GC-ing ahead of time. Change-Id: I821820a440ddb1160ba785e6656b1b242c1a72a4 --- .../google/gerrit/server/index/change/AllChangesIndexer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java index 927440e9ce..2f23ad857d 100644 --- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java +++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java @@ -242,7 +242,8 @@ public class AllChangesIndexer extends SiteIndexer Date: Sun, 14 Jun 2020 20:57:00 +0200 Subject: [PATCH 07/11] Clarify that index.batchThreads is relevant for offline reindexing The documentation did not say that the setting would not affect offline reindexing. But by putting focus on the effect on online reindexing, it somewhat suggested it. That would be misleading and might lead people to run offline reindexing with index.batchThreads a conservative 1 from a production environment and consequently leave most CPUs unnecessarily idling for offline reindexing. Change-Id: I1c9425e1f29e9320fedf7d093399aea5c73ba411 --- Documentation/config-gerrit.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 91d93c3602..e68135ecfa 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2905,7 +2905,7 @@ by the JVM. If set to a negative value, defaults to a direct executor. [[index.batchThreads]]index.batchThreads:: + Number of threads to use for indexing in background operations, such as -online schema upgrades. +online schema upgrades, and also for offline reindexing. + If not set or set to a zero, defaults to the number of logical CPUs as returned by the JVM. If set to a negative value, defaults to a direct executor. From 98b5d8f55c5f41fb7afc262c8438ac864a835726 Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Mon, 15 Jun 2020 15:56:06 +0200 Subject: [PATCH 08/11] Report end of NoteDB migration when skipping reindexing When performing an offline migration to NoteDB while skipping the reindexing step, the completion of the migration did not get reported. Since the completion is reported when migrating with reindexing, we now also report it when skipping the reindexing step. Change-Id: I75d3a8748b4719080ff42ba7715373aeafc2162a --- java/com/google/gerrit/pgm/MigrateToNoteDb.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/pgm/MigrateToNoteDb.java b/java/com/google/gerrit/pgm/MigrateToNoteDb.java index 8d66bada2a..7892abf4c9 100644 --- a/java/com/google/gerrit/pgm/MigrateToNoteDb.java +++ b/java/com/google/gerrit/pgm/MigrateToNoteDb.java @@ -161,8 +161,11 @@ public class MigrateToNoteDb extends SiteProgram { stop(); } + System.out.println("NoteDB Migration complete."); boolean reindex = firstNonNull(this.reindex, !trial); if (!reindex) { + System.out.println("Reindexing changes is skipped (either because of trial mode, or "); + System.out.println("because you requested so). If needed, reindex changes manually."); return 0; } // Reindex all indices, to save the user from having to run yet another program by hand while @@ -179,7 +182,7 @@ public class MigrateToNoteDb extends SiteProgram { reindexArgsBuilder.add("--verbose"); } List reindexArgs = reindexArgsBuilder.build(); - System.out.println("Migration complete, reindexing changes with:"); + System.out.println("Reindexing changes with:"); System.out.println(" reindex " + reindexArgs.stream().collect(joining(" "))); Reindex reindexPgm = new Reindex(); return reindexPgm.main(reindexArgs.stream().toArray(String[]::new)); From d2d5658c17a34836cec381708bbec27f5f80f31c Mon Sep 17 00:00:00 2001 From: Christian Aistleitner Date: Mon, 15 Jun 2020 16:17:17 +0200 Subject: [PATCH 09/11] Document skipping of reindexing step for offline NoteDB migration Since the reindexing of changes can take a considerable amount of time, we explicitly mention in the documentation that it can be skipped. The needed parameter is of course already described in the migration program's help page, but that's too easy to miss if the parameter is not explicitly mentioned in the documentation. Change-Id: I873f3f072c6249e618419466c3e8e16d2160d495 --- Documentation/note-db.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/note-db.txt b/Documentation/note-db.txt index 555120a5ca..509b681a62 100644 --- a/Documentation/note-db.txt +++ b/Documentation/note-db.txt @@ -109,6 +109,13 @@ Migration requires a heap size comparable to running a Gerrit server. If you normally run `gerrit.war daemon` with an `-Xmx` flag, pass that to the migration tool as well. +[NOTE] +Note that by appending `--reindex false` to the above command, you can skip the +lenghty, implicit reindexing step of the migration. This is useful if you plan +to perform further Gerrit upgrades while the server is offline and have to +reindex later anyways (E.g.: a follow-up upgrade to Gerrit 3.2 or newer, which +requires to reindex changes anyways). + *Advantages* * Much faster than online; can use all available CPUs, since no live traffic From 1f7c3856805e8d855ab68f8cfec8c14bf4237c67 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 16 Jun 2020 09:22:37 +0900 Subject: [PATCH 10/11] Fix typos in note-db.txt Change-Id: Iefecb8654567cfd38462f6de0650826c4b1f49f8 --- Documentation/note-db.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/note-db.txt b/Documentation/note-db.txt index 509b681a62..a9b71c1357 100644 --- a/Documentation/note-db.txt +++ b/Documentation/note-db.txt @@ -111,10 +111,10 @@ tool as well. [NOTE] Note that by appending `--reindex false` to the above command, you can skip the -lenghty, implicit reindexing step of the migration. This is useful if you plan +lengthy, implicit reindexing step of the migration. This is useful if you plan to perform further Gerrit upgrades while the server is offline and have to -reindex later anyways (E.g.: a follow-up upgrade to Gerrit 3.2 or newer, which -requires to reindex changes anyways). +reindex later anyway (E.g.: a follow-up upgrade to Gerrit 3.2 or newer, which +requires to reindex changes anyway). *Advantages* From 56918394544b6207a626cb0938d0bba4927a9fa4 Mon Sep 17 00:00:00 2001 From: Marcin Czech Date: Tue, 16 Jun 2020 07:47:16 +0000 Subject: [PATCH 11/11] Revert "Remove documentation of obsolete gerrit.canLoadInIFrame" This reverts commit 21d22133a1232cd71bb8fc8dcb6f806f62afee68. Reason for revert: gerrit.canLoadInIFrame is needed for Issue 12926 Change-Id: Ie7332415751496ab0f1f9b6d7ca72c05bb881903 --- Documentation/config-gerrit.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 68b16154ad..531bb94f7c 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2079,6 +2079,13 @@ file containing the class must be placed in the `$site_path/lib` folder. + If not specified, the default no-op implementation is used. +[[gerrit.canLoadInIFrame]]gerrit.canLoadInIFrame:: ++ +For security reasons Gerrit will always jump out of iframe. +Setting this option to true will prevent this behavior. ++ +By default false. + [[gerrit.cdnPath]]gerrit.cdnPath:: + Path prefix for PolyGerrit's static resources if using a CDN.