From 15f648ac2f0ff1a8c840d5b9783e9764c9d2224d Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Mon, 9 Sep 2019 10:00:28 +0200 Subject: [PATCH 1/4] NoteDbMigrator: Totally skip migration for orphan changes This case was explicitly handled by ChangeRebuilderImpl, and NoteDbMigrator in fact caught RepositoryNotFoundException and let the migration proceed. Unfortunately, this would then cause the primary storage migration step to fail with: com.google.gwtorm.server.OrmRuntimeException: change X has no note_db_state; rebuild it first Work around this by catching this exception and checking after the fact whether the change is known to be unrecoverably corrupt. There is already another check in place: no patch set, in which case the migration process would also proceed, that was added in I0439c7fc8e5. So that we are just adding yet another "supported" kind of change corruption recovery mechanism to the migration proccess. Also add a test that is trying to migrate two different changes from two different projects, where one project was deleted on the file system. That test demonstrates, that one change is completely migrated while for another change the migration is skipped, as it is detected as corrupted change. The usual use case for orphan changes, also, the changes with not existing git repsitories on the file system, is often seen for gerrit test instances: Database replicated from the production system but only a couple of git repositories are replicated, leading to orphan changes. Reported-by: Alan Tokaev Bug: Issue 12097 Change-Id: I516f40f03feaafe3014fabb0c2f5c40d6753b8bc --- .../server/notedb/rebuild/NoteDbMigrator.java | 18 +++++++-- .../notedb/OnlineNoteDbMigrationIT.java | 39 +++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java index ebec9c59df..d6daae16b2 100644 --- a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java +++ b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java @@ -72,6 +72,7 @@ import com.google.gerrit.server.notedb.PrimaryStorageMigrator.NoNoteDbStateExcep import com.google.gerrit.server.notedb.RepoSequence; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException; import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.update.ChainedReceiveCommands; import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gerrit.server.util.ManualRequestContext; @@ -164,6 +165,7 @@ public class NoteDbMigrator implements AutoCloseable { private final MutableNotesMigration globalNotesMigration; private final PrimaryStorageMigrator primaryStorageMigrator; private final DynamicSet listeners; + private final ProjectCache projectCache; private int threads; private ImmutableList projects = ImmutableList.of(); @@ -193,7 +195,8 @@ public class NoteDbMigrator implements AutoCloseable { WorkQueue workQueue, MutableNotesMigration globalNotesMigration, PrimaryStorageMigrator primaryStorageMigrator, - DynamicSet listeners) { + DynamicSet listeners, + ProjectCache projectCache) { // Reload gerrit.config/notedb.config on each migrator invocation, in case a previous // migration in the same process modified the on-disk contents. This ensures the defaults for // trial/autoMigrate get set correctly below. @@ -213,6 +216,7 @@ public class NoteDbMigrator implements AutoCloseable { this.globalNotesMigration = globalNotesMigration; this.primaryStorageMigrator = primaryStorageMigrator; this.listeners = listeners; + this.projectCache = projectCache; this.trial = getTrialMode(cfg); this.autoMigrate = getAutoMigrate(cfg); } @@ -400,6 +404,7 @@ public class NoteDbMigrator implements AutoCloseable { changes, progressOut, stopAtState, + projectCache, trial, forceRebuild, sequenceGap >= 0 ? sequenceGap : Sequences.getChangeSequenceGap(cfg), @@ -429,6 +434,7 @@ public class NoteDbMigrator implements AutoCloseable { private final ImmutableList changes; private final OutputStream progressOut; private final NotesMigrationState stopAtState; + private final ProjectCache projectCache; private final boolean trial; private final boolean forceRebuild; private final int sequenceGap; @@ -455,6 +461,7 @@ public class NoteDbMigrator implements AutoCloseable { ImmutableList changes, OutputStream progressOut, NotesMigrationState stopAtState, + ProjectCache projectCache, boolean trial, boolean forceRebuild, int sequenceGap, @@ -489,6 +496,7 @@ public class NoteDbMigrator implements AutoCloseable { this.changes = changes; this.progressOut = progressOut; this.stopAtState = stopAtState; + this.projectCache = projectCache; this.trial = trial; this.forceRebuild = forceRebuild; this.sequenceGap = sequenceGap; @@ -702,11 +710,13 @@ public class NoteDbMigrator implements AutoCloseable { * of the NoteDb migration code, which is too risky to attempt in the stable branch where this bug * had to be fixed. * - *

As of this writing, the only case where this happens is when a change has no patch sets. + *

As of this writing, there are only two cases where this happens: when a change has no patch + * sets, or the project doesn't exist. */ - private static boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) { + private boolean canSkipPrimaryStorageMigration(ReviewDb db, Change.Id id) { try { - return Iterables.isEmpty(unwrapDb(db).patchSets().byChange(id)); + return Iterables.isEmpty(unwrapDb(db).patchSets().byChange(id)) + || projectCache.get(unwrapDb(db).changes().get(id).getProject()) == null; } catch (Exception e) { logger.atSevere().withCause(e).log( "Error checking if change %s can be skipped, assuming no", id); diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java index c2499737d9..b9ed0f3ac7 100644 --- a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java +++ b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java @@ -35,6 +35,8 @@ import static org.easymock.EasyMock.verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.io.MoreFiles; +import com.google.common.io.RecursiveDeleteOption; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.NoHttpd; @@ -84,6 +86,7 @@ import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.RepositoryCache.FileKey; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.util.FS; import org.junit.After; @@ -510,6 +513,42 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest { } } + @Test + public void fullMigrationOneChangeWithNoProject() throws Exception { + PushOneCommit.Result r1 = createChange(); + Change.Id id1 = r1.getChange().getId(); + + Project.NameKey p2 = createProject("project2"); + TestRepository tr2 = cloneProject(p2, admin); + PushOneCommit.Result r2 = pushFactory.create(db, admin.getIdent(), tr2).to("refs/for/master"); + Change.Id id2 = r2.getChange().getId(); + + // TODO(davido): Find an easier way to wipe out a repository from the file system. + MoreFiles.deleteRecursively( + FileKey.lenient( + sitePaths + .resolve(cfg.getString("gerrit", null, "basePath")) + .resolve(p2.get()) + .toFile(), + FS.DETECTED) + .getFile() + .toPath(), + RecursiveDeleteOption.ALLOW_INSECURE); + + migrate(b -> b); + assertNotesMigrationState(NOTE_DB, false, false); + + try (ReviewDb db = schemaFactory.open(); + Repository repo = repoManager.openRepository(project)) { + assertThat(repo.exactRef(RefNames.changeMetaRef(id1))).isNotNull(); + assertThat(db.changes().get(id1).getNoteDbState()).isEqualTo(NOTE_DB_PRIMARY_STATE); + } + + // A change without project is so corrupt that it is completely skipped by the migration + // process. + assertThat(db.changes().get(id2).getNoteDbState()).isNull(); + } + @Test public void fullMigrationMissingPatchSetRefs() throws Exception { PushOneCommit.Result r = createChange(); From 5f948ecf1de2195e479513db0d8a40d45f635410 Mon Sep 17 00:00:00 2001 From: Thomas Draebing Date: Mon, 20 Jan 2020 11:47:12 +0100 Subject: [PATCH 2/4] Fix commentlinks with same prefix in pattern and link If using the commentlinks.%NAME%.links option and having the same prefix in the pattern and the url, the parsing did not work properly. For example: [commentlink "test"] pattern = [Hh][Tt][Tt][Pp]example link = http://example.com and the line in the message: httpexample 1234 would result in: httphttpexample 1234 This bug was introduced in a change implementing the possibility to have overlapping patterns, e.g. allowing a comma-separated list of issue ids. (commit: fb48902ef303d008c6f0dd38d654106c00313530). This would have however anyway only worked for the `html`-variant of comment links, which allow to directly provide html-elements as a substitute for the text matching the pattern. Thus, this change restricts the logic introduced in the above mentioned commit to the html-variant. Bug: Issue 12197 Change-Id: I9efd9308f2b40f3fec8405c93849d13d4576562a --- .../gr-linked-text/gr-linked-text_test.html | 16 ++++++++++++++++ .../shared/gr-linked-text/link-text-parser.js | 19 ++++++++++--------- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html index 6d413b7fab..19839a8c57 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html +++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html @@ -50,6 +50,10 @@ limitations under the License. match: '([Bb]ug|[Ii]ssue)\\s*#?(\\d+)', link: 'https://bugs.chromium.org/p/gerrit/issues/detail?id=$2', }, + prefixsameinlinkandpattern: { + match: '([Hh][Tt][Tt][Pp]example)\\s*#?(\\d+)', + link: 'https://bugs.chromium.org/p/gerrit/issues/detail?id=$2', + }, changeid: { match: '(I[0-9a-f]{8,40})', link: '#/q/$1', @@ -116,6 +120,18 @@ limitations under the License. assert.equal(linkEl.textContent, 'Bug 3650'); }); + test('Pattern with same prefix as link was correctly parsed', () => { + // Pattern starts with the same prefix (`http`) as the url. + element.content = 'httpexample 3650'; + + assert.equal(element.$.output.childNodes.length, 1); + const linkEl = element.$.output.childNodes[0]; + const url = 'https://bugs.chromium.org/p/gerrit/issues/detail?id=3650'; + assert.equal(linkEl.target, '_blank'); + assert.equal(linkEl.href, url); + assert.equal(linkEl.textContent, 'httpexample 3650'); + }); + test('Change-Id pattern was parsed and linked', () => { // "Change-Id:" pattern. const changeID = 'I11d6a37f5e9b5df0486f6c922d8836dfa780e03e'; diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js b/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js index 23a71f99e3..027c632e12 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js +++ b/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js @@ -312,14 +312,15 @@ let result = match[0].replace(pattern, patterns[p].html || patterns[p].link); - let i; - // Skip portion of replacement string that is equal to original. - for (i = 0; i < result.length; i++) { - if (result[i] !== match[0][i]) { break; } - } - result = result.slice(i); - if (patterns[p].html) { + let i; + // Skip portion of replacement string that is equal to original to + // allow overlapping patterns. + for (i = 0; i < result.length; i++) { + if (result[i] !== match[0][i]) { break; } + } + result = result.slice(i); + this.addHTML( result, susbtrIndex + match.index + i, @@ -329,8 +330,8 @@ this.addLink( match[0], result, - susbtrIndex + match.index + i, - match[0].length - i, + susbtrIndex + match.index, + match[0].length, outputArray); } else { throw Error('linkconfig entry ' + p + From ae4a73b87c27288ee0d9de18acc2637c9ae0ef12 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 31 Jan 2020 10:37:47 +0100 Subject: [PATCH 3/4] Improve Jetty thread pool metrics Follow-up change for be4184f7 which missed documentation. Also names of newly added metrics didn't align with existing metrics for REST API. - add documentation for the jetty metrics - improve descriptions of the jetty metrics - rename jetty thread pool metrics from httpd/jetty/* to http/server/jetty/* in order to align metric names with already existing REST API metrics Change-Id: I1d4eabec91749f28d09df7d2d29606b133fefc4d --- Documentation/metrics.txt | 7 ++++++ .../gerrit/pgm/http/jetty/JettyMetrics.java | 24 +++++++++---------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index 37d6d01c85..1a756d1f04 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -56,6 +56,13 @@ objects needing finalization. === HTTP +* `http/server/jetty/threadpool/min_pool_size`: Minimum thread pool size +* `http/server/jetty/threadpool/max_pool_size`: Maximum thread pool size +* `http/server/jetty/threadpool/pool_size`: Current thread pool size +* `http/server/jetty/threadpool/idle_threads`: Idle threads +* `http/server/jetty/threadpool/active_threads`: Active threads +* `http/server/jetty/threadpool/reserved_threads`: Reserved threads +* `http/server/jetty/threadpool/queue_size`: Queued requests waiting for a thread * `http/server/error_count`: Rate of REST API error responses. * `http/server/success_count`: Rate of REST API success responses. * `http/server/rest_api/count`: Rate of REST API calls by view. diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java b/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java index 92edf403d1..b6a2d38710 100644 --- a/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java +++ b/java/com/google/gerrit/pgm/http/jetty/JettyMetrics.java @@ -28,42 +28,42 @@ public class JettyMetrics { JettyMetrics(JettyServer jetty, MetricMaker metrics) { CallbackMetric0 minPoolSize = metrics.newCallbackMetric( - "httpd/jetty/threadpool/min_pool_size", + "http/server/jetty/threadpool/min_pool_size", Integer.class, new Description("Minimum thread pool size").setGauge()); CallbackMetric0 maxPoolSize = metrics.newCallbackMetric( - "httpd/jetty/threadpool/max_pool_size", + "http/server/jetty/threadpool/max_pool_size", Integer.class, new Description("Maximum thread pool size").setGauge()); CallbackMetric0 poolSize = metrics.newCallbackMetric( - "httpd/jetty/threadpool/pool_size", + "http/server/jetty/threadpool/pool_size", Integer.class, new Description("Current thread pool size").setGauge()); CallbackMetric0 idleThreads = metrics.newCallbackMetric( - "httpd/jetty/threadpool/idle_threads", + "http/server/jetty/threadpool/idle_threads", Integer.class, - new Description("Idle httpd threads").setGauge().setUnit("threads")); + new Description("Idle threads").setGauge().setUnit("threads")); CallbackMetric0 busyThreads = metrics.newCallbackMetric( - "httpd/jetty/threadpool/active_threads", + "http/server/jetty/threadpool/active_threads", Integer.class, - new Description("Active httpd threads").setGauge().setUnit("threads")); + new Description("Active threads").setGauge().setUnit("threads")); CallbackMetric0 reservedThreads = metrics.newCallbackMetric( - "httpd/jetty/threadpool/reserved_threads", + "http/server/jetty/threadpool/reserved_threads", Integer.class, - new Description("Reserved httpd threads").setGauge().setUnit("threads")); + new Description("Reserved threads").setGauge().setUnit("threads")); CallbackMetric0 queueSize = metrics.newCallbackMetric( - "httpd/jetty/threadpool/queue_size", + "http/server/jetty/threadpool/queue_size", Integer.class, - new Description("Thread pool queue size").setGauge().setUnit("requests")); + new Description("Queued requests waiting for a thread").setGauge().setUnit("requests")); CallbackMetric0 lowOnThreads = metrics.newCallbackMetric( - "httpd/jetty/threadpool/is_low_on_threads", + "http/server/jetty/threadpool/is_low_on_threads", Boolean.class, new Description("Whether thread pool is low on threads").setGauge()); JettyServer.Metrics jettyMetrics = jetty.getMetrics(); From db1e78649e32296bbe604e6d67fdd75bbb4991b6 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 31 Jan 2020 10:37:47 +0100 Subject: [PATCH 4/4] Move documentation for Jetty metrics to a separate section Change-Id: Ibdb1516b091c81203c282384dbe8aa2276c838ff --- Documentation/metrics.txt | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index 1a756d1f04..ce9f74ee8c 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -56,13 +56,18 @@ objects needing finalization. === HTTP -* `http/server/jetty/threadpool/min_pool_size`: Minimum thread pool size -* `http/server/jetty/threadpool/max_pool_size`: Maximum thread pool size -* `http/server/jetty/threadpool/pool_size`: Current thread pool size -* `http/server/jetty/threadpool/idle_threads`: Idle threads +==== Jetty + * `http/server/jetty/threadpool/active_threads`: Active threads +* `http/server/jetty/threadpool/idle_threads`: Idle threads * `http/server/jetty/threadpool/reserved_threads`: Reserved threads +* `http/server/jetty/threadpool/max_pool_size`: Maximum thread pool size +* `http/server/jetty/threadpool/min_pool_size`: Minimum thread pool size +* `http/server/jetty/threadpool/pool_size`: Current thread pool size * `http/server/jetty/threadpool/queue_size`: Queued requests waiting for a thread + +==== REST API + * `http/server/error_count`: Rate of REST API error responses. * `http/server/success_count`: Rate of REST API success responses. * `http/server/rest_api/count`: Rate of REST API calls by view.