From 3905f5527103d218502d1f817b26a41e195b2aae Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 5 Feb 2018 12:08:21 +0900 Subject: [PATCH 01/10] Set version to 2.13.10 Change-Id: I9c0878e4a838d19bb257146639dc5da1c6958d3e --- Documentation/dev-plugins.txt | 2 +- VERSION | 2 +- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-archetype/pom.xml | 2 +- gerrit-plugin-gwt-archetype/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-plugin-js-archetype/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index d1a34928dc..3716c40695 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -36,7 +36,7 @@ plugin project. ---- mvn archetype:generate -DarchetypeGroupId=com.google.gerrit \ -DarchetypeArtifactId=gerrit-plugin-archetype \ - -DarchetypeVersion=2.13.9 \ + -DarchetypeVersion=2.13.10 \ -DgroupId=com.googlesource.gerrit.plugins.testplugin \ -DartifactId=testplugin ---- diff --git a/VERSION b/VERSION index 3a7cb77bd1..e31db7367d 100644 --- a/VERSION +++ b/VERSION @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = '2.13.9' +GERRIT_VERSION = '2.13.10' diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index d3a4c58bfe..68f98b1e98 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.13.9 + 2.13.10 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 73ba5ae2a1..144031b713 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.13.9 + 2.13.10 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index 0730521255..504d53f0c6 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.13.9 + 2.13.10 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-archetype/pom.xml b/gerrit-plugin-archetype/pom.xml index 80e7a4b1e5..a8794da312 100644 --- a/gerrit-plugin-archetype/pom.xml +++ b/gerrit-plugin-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-archetype - 2.13.9 + 2.13.10 Gerrit Code Review - Plugin Archetype Maven Archetype for Gerrit Plugins https://www.gerritcodereview.com/ diff --git a/gerrit-plugin-gwt-archetype/pom.xml b/gerrit-plugin-gwt-archetype/pom.xml index b85e9658f9..3b052bf547 100644 --- a/gerrit-plugin-gwt-archetype/pom.xml +++ b/gerrit-plugin-gwt-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-gwt-archetype - 2.13.9 + 2.13.10 Gerrit Code Review - Web UI GWT Plugin Archetype Maven Archetype for Gerrit Web UI GWT Plugins https://www.gerritcodereview.com/ diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index 7643c8a9fe..728c4b34d4 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.13.9 + 2.13.10 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-plugin-js-archetype/pom.xml b/gerrit-plugin-js-archetype/pom.xml index 3f1f13979d..4140da4b11 100644 --- a/gerrit-plugin-js-archetype/pom.xml +++ b/gerrit-plugin-js-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-js-archetype - 2.13.9 + 2.13.10 Gerrit Code Review - Web UI JavaScript Plugin Archetype Maven Archetype for Gerrit Web UI JavaScript Plugins https://www.gerritcodereview.com/ diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index c26268caba..eef6b87974 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.13.9 + 2.13.10 war Gerrit Code Review - WAR Gerrit WAR From 6b8e53fd66bedc65337a8fa7bd0e6973f5b90735 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 9 Feb 2018 12:28:00 -0500 Subject: [PATCH 02/10] NoteDbMigrator: Actually close ReviewDb after scanning projects The code claims it was doing this, but it wasn't, unless --threads=1 and all work is done in the main thread. Change it to explicitly open and close the DB. This results in an extra open/close in the single-thread case, but that's negligible compared to the cost of rebuilding. Change-Id: Ic05b0118c2589ff1c8205b1140d11fe1f3d946a6 --- .../server/notedb/rebuild/NoteDbMigrator.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java index c8d69b9b80..63d9841ef2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java @@ -691,10 +691,9 @@ public class NoteDbMigrator implements AutoCloseable { Stopwatch sw = Stopwatch.createStarted(); log.info("Rebuilding changes in NoteDb"); + ImmutableListMultimap changesByProject = getChangesByProject(); List> futures = new ArrayList<>(); try (ContextHelper contextHelper = new ContextHelper()) { - ImmutableListMultimap changesByProject = - getChangesByProject(contextHelper.getReviewDb()); List projectNames = Ordering.usingToString().sortedCopy(changesByProject.keySet()); for (Project.NameKey project : projectNames) { @@ -723,7 +722,7 @@ public class NoteDbMigrator implements AutoCloseable { } } - private ImmutableListMultimap getChangesByProject(ReviewDb db) + private ImmutableListMultimap getChangesByProject() throws OrmException { // Memoize all changes so we can close the db connection and allow other threads to use the full // connection pool. @@ -731,13 +730,15 @@ public class NoteDbMigrator implements AutoCloseable { MultimapBuilder.treeKeys(comparing(Project.NameKey::get)) .treeSetValues(comparing(Change.Id::get)) .build(); - if (!projects.isEmpty()) { - return byProject(db.changes().all(), c -> projects.contains(c.getProject()), out); + try (ReviewDb db = unwrapDb(schemaFactory.open())) { + if (!projects.isEmpty()) { + return byProject(db.changes().all(), c -> projects.contains(c.getProject()), out); + } + if (!changes.isEmpty()) { + return byProject(db.changes().get(changes), c -> true, out); + } + return byProject(db.changes().all(), c -> true, out); } - if (!changes.isEmpty()) { - return byProject(db.changes().get(changes), c -> true, out); - } - return byProject(db.changes().all(), c -> true, out); } private static ImmutableListMultimap byProject( From 076716daaf7570a3e957e48ea731c87058b26744 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 9 Feb 2018 10:28:10 -0500 Subject: [PATCH 03/10] NoteDbMigrator: Avoid holding pending change operations in memory The previous approach used a single NoteDbUpdateManager to keep track of all change operations, which were then flushed and executed in a single operation (execute()). This is conceptually simple, but has a major downside of storing in memory a Java representation of every modification to every change in a project. This is because a NoteDbUpdateManager is a glorified map of change to lists of ChangeUpdates, and each ChangeUpdate needs to store the full information about each modification. From a simple profile of migrating gerrit-review, this expensive approach costs around 2 KiB of heap per operation, which can add up quickly, costing several GiB to migrate just the gerrit project. Gerrit only has 25k or so changes; this scaling would be a nonstarter on a project with hundreds of thousands of changes. Instead of using a single NoteDbUpdateManager, use one manager per change, and have them flush to a shared ObjectInserter and ChainedReceiveCommands. This means we can close the manager immediately after rebuilding a single change, freeing the ChangeUpdates that would take up memory. The only data we need to buffer in memory now is the internal buffer of the PackInserter, which frequently flushes itself to disk, and the list of pending ReceiveCommands. This implementation does some sketchy things, like copying commands between different ChainedReceiveCommands instances and abusing the dryRun argument to NoteDbUpdateManager#execute. But it works, and I'd rather have some ugliness isolated to NoteDbMigrator than complicate the NoteDbUpdateManager interface or implementation further. The manager code will live longer than the migrator, and is more important for normal operation of NoteDb, so more risky to change in a stable branch. Bug: Issue 8046 Change-Id: I86849c1fade947731c0e51de2ce58598777e82c5 --- .../server/notedb/NoteDbUpdateManager.java | 1 - .../server/notedb/rebuild/NoteDbMigrator.java | 154 ++++++++++++------ 2 files changed, 105 insertions(+), 50 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java index 3e36de93bb..b9f5fe6be8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java @@ -619,7 +619,6 @@ public class NoteDbUpdateManager implements AutoCloseable { } else { // OpenRepo buffers objects separately; caller may assume that objects are available in the // inserter it previously passed via setChangeRepo. - checkState(saveObjects, "cannot use dryrun with saveObjects = false"); or.flushToFinalInserter(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java index 63d9841ef2..8e8f232c2b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java @@ -48,9 +48,11 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbWrapper; +import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.config.AllProjectsName; +import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerConfigProvider; import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; @@ -68,11 +70,13 @@ 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.update.ChainedReceiveCommands; +import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gerrit.server.util.ManualRequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; +import com.google.inject.Provider; import java.io.BufferedWriter; import java.io.IOException; import java.io.OutputStream; @@ -80,10 +84,8 @@ import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.util.ArrayList; import java.util.Collection; -import java.util.HashSet; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; @@ -92,14 +94,17 @@ import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.internal.storage.file.PackInserter; +import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.TextProgressMonitor; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.io.NullOutputStream; import org.slf4j.Logger; @@ -131,6 +136,8 @@ public class NoteDbMigrator implements AutoCloseable { public static class Builder { private final Config cfg; private final SitePaths sitePaths; + private final Provider serverIdent; + private final AllUsersName allUsers; private final SchemaFactory schemaFactory; private final GitRepositoryManager repoManager; private final NoteDbUpdateManager.Factory updateManagerFactory; @@ -158,6 +165,8 @@ public class NoteDbMigrator implements AutoCloseable { Builder( GerritServerConfigProvider configProvider, SitePaths sitePaths, + @GerritPersonIdent Provider serverIdent, + AllUsersName allUsers, SchemaFactory schemaFactory, GitRepositoryManager repoManager, NoteDbUpdateManager.Factory updateManagerFactory, @@ -175,6 +184,8 @@ public class NoteDbMigrator implements AutoCloseable { // trial/autoMigrate get set correctly below. this.cfg = configProvider.get(); this.sitePaths = sitePaths; + this.serverIdent = serverIdent; + this.allUsers = allUsers; this.schemaFactory = schemaFactory; this.repoManager = repoManager; this.updateManagerFactory = updateManagerFactory; @@ -337,6 +348,8 @@ public class NoteDbMigrator implements AutoCloseable { return new NoteDbMigrator( sitePaths, schemaFactory, + serverIdent, + allUsers, repoManager, updateManagerFactory, bundleReader, @@ -364,6 +377,8 @@ public class NoteDbMigrator implements AutoCloseable { private final FileBasedConfig gerritConfig; private final FileBasedConfig noteDbConfig; private final SchemaFactory schemaFactory; + private final Provider serverIdent; + private final AllUsersName allUsers; private final GitRepositoryManager repoManager; private final NoteDbUpdateManager.Factory updateManagerFactory; private final ChangeBundleReader bundleReader; @@ -388,6 +403,8 @@ public class NoteDbMigrator implements AutoCloseable { private NoteDbMigrator( SitePaths sitePaths, SchemaFactory schemaFactory, + Provider serverIdent, + AllUsersName allUsers, GitRepositoryManager repoManager, NoteDbUpdateManager.Factory updateManagerFactory, ChangeBundleReader bundleReader, @@ -416,6 +433,8 @@ public class NoteDbMigrator implements AutoCloseable { } this.schemaFactory = schemaFactory; + this.serverIdent = serverIdent; + this.allUsers = allUsers; this.rebuilder = rebuilder; this.repoManager = repoManager; this.updateManagerFactory = updateManagerFactory; @@ -768,59 +787,63 @@ public class NoteDbMigrator implements AutoCloseable { new TextProgressMonitor( new PrintWriter(new BufferedWriter(new OutputStreamWriter(progressOut, UTF_8)))); try (Repository changeRepo = repoManager.openRepository(project); + // Only use a PackInserter for the change repo, not All-Users. + // + // It's not possible to share a single inserter for All-Users across all project tasks, and + // we don't want to add one pack per project to All-Users. Adding many loose objects is + // preferable to many packs. + // + // Anyway, the number of objects inserted into All-Users is proportional to the number + // of pending draft comments, which should not be high (relative to the total number of + // changes), so the number of loose objects shouldn't be too unreasonable. ObjectInserter changeIns = newPackInserter(changeRepo); ObjectReader changeReader = changeIns.newReader(); RevWalk changeRw = new RevWalk(changeReader); - NoteDbUpdateManager manager = - updateManagerFactory - .create(project) - .setSaveObjects(false) - .setAtomicRefUpdates(false) - // Only use a PackInserter for the change repo, not All-Users. - // - // It's not possible to share a single inserter for All-Users across all project - // tasks, and we don't want to add one pack per project to All-Users. Adding many - // loose objects is preferable to many packs. - // - // Anyway, the number of objects inserted into All-Users is proportional to the - // number of pending draft comments, which should not be high (relative to the total - // number of changes), so the number of loose objects shouldn't be too unreasonable. - .setChangeRepo( - changeRepo, changeRw, changeIns, new ChainedReceiveCommands(changeRepo))) { - Set skipExecute = new HashSet<>(); + Repository allUsersRepo = repoManager.openRepository(allUsers); + ObjectInserter allUsersIns = allUsersRepo.newObjectInserter(); + ObjectReader allUsersReader = allUsersIns.newReader(); + RevWalk allUsersRw = new RevWalk(allUsersReader)) { + ChainedReceiveCommands changeCmds = new ChainedReceiveCommands(changeRepo); + ChainedReceiveCommands allUsersCmds = new ChainedReceiveCommands(allUsersRepo); + Collection changes = allChanges.get(project); pm.beginTask(FormatUtil.elide("Rebuilding " + project.get(), 50), changes.size()); + int toSave = 0; try { for (Change.Id changeId : changes) { - boolean staged = false; - try { - stage(db, changeId, manager); - staged = true; + // NoteDbUpdateManager assumes that all commands in its OpenRepo were added by itself, so + // we can't share the top-level ChainedReceiveCommands. Use a new set of commands sharing + // the same underlying repo, and copy commands back to the top-level + // ChainedReceiveCommands later. This also assumes that each ref in the final list of + // commands was only modified by a single NoteDbUpdateManager; since we use one manager + // per change, and each ref corresponds to exactly one change, this assumption should be + // safe. + ChainedReceiveCommands tmpChangeCmds = + new ChainedReceiveCommands(changeCmds.getRepoRefCache()); + ChainedReceiveCommands tmpAllUsersCmds = + new ChainedReceiveCommands(allUsersCmds.getRepoRefCache()); + + try (NoteDbUpdateManager manager = + updateManagerFactory + .create(project) + .setAtomicRefUpdates(false) + .setSaveObjects(false) + .setChangeRepo(changeRepo, changeRw, changeIns, tmpChangeCmds) + .setAllUsersRepo(allUsersRepo, allUsersRw, allUsersIns, tmpAllUsersCmds)) { + rebuild(db, changeId, manager); + + // Executing with dryRun=true writes all objects to the underlying inserters and adds + // commands to the ChainedReceiveCommands. Afterwards, we can discard the manager, so we + // don't keep using any memory beyond what may be buffered in the PackInserter. + manager.execute(true); + + tmpChangeCmds.getCommands().values().forEach(c -> addCommand(changeCmds, c)); + tmpAllUsersCmds.getCommands().values().forEach(c -> addCommand(allUsersCmds, c)); + + toSave++; } catch (NoPatchSetsException e) { log.warn(e.getMessage()); - } catch (Throwable t) { - log.error("Failed to rebuild change " + changeId, t); - ok = false; - } - pm.update(1); - if (!staged) { - skipExecute.add(changeId); - } - } - } finally { - pm.endTask(); - } - - pm.beginTask( - FormatUtil.elide("Saving " + project.get(), 50), changes.size() - skipExecute.size()); - try { - for (Change.Id changeId : changes) { - if (skipExecute.contains(changeId)) { - continue; - } - try { - rebuilder.execute(db, changeId, manager, true, false); - } catch (ConflictingUpdateException e) { + } catch (ConflictingUpdateException ex) { log.warn( "Rebuilding detected a conflicting ReviewDb update for change {};" + " will be auto-rebuilt at runtime", @@ -835,15 +858,23 @@ public class NoteDbMigrator implements AutoCloseable { pm.endTask(); } + pm.beginTask(FormatUtil.elide("Saving " + project.get(), 50), ProgressMonitor.UNKNOWN); try { - manager.execute(); + save(changeRepo, changeRw, changeIns, changeCmds); + save(allUsersRepo, allUsersRw, allUsersIns, allUsersCmds); + // This isn't really useful progress. If we passed a real ProgressMonitor to + // BatchRefUpdate#execute we might get something more incremental, but that doesn't allow us + // to specify the repo name in the task text. + pm.update(toSave); } catch (LockFailureException e) { log.warn( "Rebuilding detected a conflicting NoteDb update for the following refs, which will" + " be auto-rebuilt at runtime: {}", e.getFailedRefs().stream().distinct().sorted().collect(joining(", "))); - } catch (OrmException | IOException e) { + } catch (IOException e) { log.error("Failed to save NoteDb state for " + project, e); + } finally { + pm.endTask(); } } catch (RepositoryNotFoundException e) { log.warn("Repository {} not found", project); @@ -853,7 +884,7 @@ public class NoteDbMigrator implements AutoCloseable { return ok; } - private void stage(ReviewDb db, Change.Id changeId, NoteDbUpdateManager manager) + private void rebuild(ReviewDb db, Change.Id changeId, NoteDbUpdateManager manager) throws OrmException, IOException { // Match ChangeRebuilderImpl#stage, but without calling manager.stage(), since that can only be // called after building updates for all changes. @@ -864,6 +895,31 @@ public class NoteDbMigrator implements AutoCloseable { throw new NoSuchChangeException(changeId); } rebuilder.buildUpdates(manager, bundleReader.fromReviewDb(db, changeId)); + + rebuilder.execute(db, changeId, manager, true, false); + } + + private static void addCommand(ChainedReceiveCommands cmds, ReceiveCommand cmd) { + // ChainedReceiveCommands doesn't allow no-ops, but these occur when rebuilding a + // previously-rebuilt change. + if (!cmd.getOldId().equals(cmd.getNewId())) { + cmds.add(cmd); + } + } + + private void save(Repository repo, RevWalk rw, ObjectInserter ins, ChainedReceiveCommands cmds) + throws IOException { + if (cmds.isEmpty()) { + return; + } + ins.flush(); + BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate(); + bru.setRefLogMessage("Migrate changes to NoteDb", false); + bru.setRefLogIdent(serverIdent.get()); + bru.setAtomic(false); + bru.setAllowNonFastForwards(true); + cmds.addTo(bru); + RefUpdateUtil.executeChecked(bru, rw); } private static boolean futuresToBoolean(List> futures, String errMsg) { From f86e24be060d2504ee0c7cef520457a5361e1b40 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 9 Feb 2018 13:15:06 -0500 Subject: [PATCH 04/10] MigrateToNoteDb: Hackily limit to 4 threads by default for H2 Bug: Issue 8022 Change-Id: I87112209ee99240c93fa19f6ef1e892bd55d6683 --- .../google/gerrit/pgm/MigrateToNoteDb.java | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java index c54445285d..9e9f1c8d4e 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/MigrateToNoteDb.java @@ -36,6 +36,7 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.index.IndexModule; import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; import com.google.gerrit.server.notedb.rebuild.NoteDbMigrator; +import com.google.gerrit.server.schema.DataSourceType; import com.google.inject.Inject; import com.google.inject.Injector; import com.google.inject.Module; @@ -50,8 +51,10 @@ public class MigrateToNoteDb extends SiteProgram { "Trial mode: migrate changes and turn on reading from NoteDb, but leave ReviewDb as the" + " source of truth"; + private static final int ISSUE_8022_THREAD_LIMIT = 4; + @Option(name = "--threads", usage = "Number of threads to use for rebuilding NoteDb") - private int threads = Runtime.getRuntime().availableProcessors(); + private Integer threads; @Option( name = "--project", @@ -109,12 +112,13 @@ public class MigrateToNoteDb extends SiteProgram { try { mustHaveValidSite(); dbInjector = createDbInjector(MULTI_USER); - threads = ThreadLimiter.limitThreads(dbInjector, threads); dbManager = new LifecycleManager(); dbManager.add(dbInjector); dbManager.start(); + threads = limitThreads(); + sysInjector = createSysInjector(); sysInjector.injectMembers(this); sysManager = new LifecycleManager(); @@ -162,6 +166,27 @@ public class MigrateToNoteDb extends SiteProgram { return reindexPgm.main(reindexArgs.stream().toArray(String[]::new)); } + private int limitThreads() { + if (threads != null) { + return threads; + } + int actualThreads; + int procs = Runtime.getRuntime().availableProcessors(); + DataSourceType dsType = dbInjector.getInstance(DataSourceType.class); + if (dsType.getDriver().equals("org.h2.Driver") && procs > ISSUE_8022_THREAD_LIMIT) { + System.out.println( + "Not using more than " + + ISSUE_8022_THREAD_LIMIT + + " threads due to http://crbug.com/gerrit/8022"); + System.out.println("Can be increased by passing --threads, but may cause errors"); + actualThreads = ISSUE_8022_THREAD_LIMIT; + } else { + actualThreads = procs; + } + actualThreads = ThreadLimiter.limitThreads(dbInjector, actualThreads); + return actualThreads; + } + private Injector createSysInjector() { return dbInjector.createChildInjector( new FactoryModule() { From 03ead35c034cd712cc398db3734c87cfe1a258aa Mon Sep 17 00:00:00 2001 From: Inderjot Kaur Ratol Date: Mon, 12 Feb 2018 11:09:15 -0500 Subject: [PATCH 05/10] Batch index executor: make use of the value passed for threads requested Previously, the batch index executor was always configured using the values provided in gerrit.config for key "batchThreads". This did not align with the expectations when using "--threads" option from command line in case of reindexing. With this change, it takes into account the value passed in the threads parameter, if no value is passed, it falls back to the value configured in gerrit.config. As the constructor with "threads" parameter is only used for reindex and DummyIndexModule for a slave, it is safe to make use of the "threads" parameter to configure the executor. Bug: Issue 8319 Change-Id: I6f26cc516e1879e710d89b7ab7d9d668d1e1a86f --- .../com/google/gerrit/server/index/IndexModule.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexModule.java index a1129b09e7..90ac602e1d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IndexModule.java @@ -192,11 +192,14 @@ public class IndexModule extends LifecycleModule { if (batchExecutor != null) { return batchExecutor; } - int threads = config.getInt("index", null, "batchThreads", 0); - if (threads <= 0) { - threads = Runtime.getRuntime().availableProcessors(); + int batchThreads = this.threads; + if (batchThreads <= 0) { + batchThreads = config.getInt("index", null, "batchThreads", 0); } - return MoreExecutors.listeningDecorator(workQueue.createQueue(threads, "Index-Batch")); + if (batchThreads <= 0) { + batchThreads = Runtime.getRuntime().availableProcessors(); + } + return MoreExecutors.listeningDecorator(workQueue.createQueue(batchThreads, "Index-Batch")); } @Singleton From b4cd3067e05b9324a4961f2765bf37d1c98a3b25 Mon Sep 17 00:00:00 2001 From: Logan Hanks Date: Tue, 2 May 2017 09:46:50 -0700 Subject: [PATCH 06/10] Disable target="_blank" on MyMenu items Each item of the My menu (called Your in PolyGerrit) is configurable by the user. There is a target property on each item, but currently no UI by which the user can set it. Therefore generally we can expect menu items to receive the default value for this property. Unfortunately, the default value is contingent on whether the server thinks the URL is a view in the GWT UI (by looking for a leading '#'). PolyGerrit URLs do not conform to this expectation, so our default menu items are typically presented by the server as pop-out links. Previously this wasn't a concern because the top menu didn't support pop-out links. We introduced support for this with the Documentation menu in https://gerrit-review.googlesource.com/c/104393/. Bug: Issue 6109 Bug: Issue 8373 Change-Id: I2455bca5ca6250939ff0a9c3b62679aa6d24dadb (cherry picked from commit 26bdf6bab524fde3544a68cecc822e8fa6aa45e3) --- .../elements/core/gr-main-header/gr-main-header.js | 14 ++++++++++++-- .../core/gr-main-header/gr-main-header_test.html | 6 ++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js index affa8e599d..ab042c465a 100644 --- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js +++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js @@ -244,7 +244,7 @@ this.$.restAPI.getPreferences().then(function(prefs) { this._userLinks = - prefs.my.map(this._stripHashPrefix).filter(this._isSupportedLink); + prefs.my.map(this._fixMyMenuItem).filter(this._isSupportedLink); }.bind(this)); this._loadAccountCapabilities(); }, @@ -260,10 +260,20 @@ }.bind(this)); }, - _stripHashPrefix: function(linkObj) { + _fixMyMenuItem: function(linkObj) { + // Normalize all urls to PolyGerrit style. if (linkObj.url.indexOf('#') === 0) { linkObj.url = linkObj.url.slice(1); } + + // Delete target property due to complications of + // https://bugs.chromium.org/p/gerrit/issues/detail?id=5888 + // + // The server tries to guess whether URL is a view within the UI. + // If not, it sets target='_blank' on the menu item. The server + // makes assumptions that work for the GWT UI, but not PolyGerrit, + // so we'll just disable it altogether for now. + delete linkObj.target; return linkObj; }, diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html index 4582b4f9c2..94f1f66f8d 100644 --- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html +++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html @@ -53,14 +53,16 @@ limitations under the License. sandbox.restore(); }); - test('strip hash prefix', function() { + test('fix my menu item', function() { assert.deepEqual([ {url: '#/q/owner:self+is:draft'}, {url: 'https://awesometown.com/#hashyhash'}, - ].map(element._stripHashPrefix), + {url: 'url', target: '_blank'}, + ].map(element._fixMyMenuItem), [ {url: '/q/owner:self+is:draft'}, {url: 'https://awesometown.com/#hashyhash'}, + {url: 'url'}, ]); }); From 00635a9928cb0bcc83f6959b68da63204d1b853c Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Wed, 6 Sep 2017 17:37:22 -0700 Subject: [PATCH 07/10] Mark all "My" menu links as external Custom menu links may use URLs that are not covered by the client router. When following a link that shares the domain, it will be intercepted by the catchall route and a visual 404 will be mistakenly shown. With this change, all of the custom links in this menu are marked as external so that following them causes a full reload and a 404 is shown only in the appropriate case. Bug: Issue 8373 Change-Id: I1cad36de97786a0f9af42e08105ab485fa198a15 --- .../app/elements/core/gr-main-header/gr-main-header.js | 6 ++++++ .../elements/core/gr-main-header/gr-main-header_test.html | 6 +++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js index ab042c465a..3e7c82ab4d 100644 --- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js +++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.js @@ -274,6 +274,12 @@ // makes assumptions that work for the GWT UI, but not PolyGerrit, // so we'll just disable it altogether for now. delete linkObj.target; + + // Becasue the "my menu" links may be arbitrary URLs, we don't know + // whether they correspond to any client routes. Mark all such links as + // external. + linkObj.external = true; + return linkObj; }, diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html index 94f1f66f8d..bea57361b9 100644 --- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html +++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.html @@ -60,9 +60,9 @@ limitations under the License. {url: 'url', target: '_blank'}, ].map(element._fixMyMenuItem), [ - {url: '/q/owner:self+is:draft'}, - {url: 'https://awesometown.com/#hashyhash'}, - {url: 'url'}, + {url: '/q/owner:self+is:draft', external: true}, + {url: 'https://awesometown.com/#hashyhash', external: true}, + {url: 'url', external: true}, ]); }); From 1abb73e39e70182cb7121fbe58e6a8567128a3b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 15 Feb 2018 11:51:51 +0100 Subject: [PATCH 08/10] Fix account_patch_reviews for mysql Creation of the account_patch_reviews table failed on mysql with the file_name column being set to varchar(4096), see [1]. mysql has a limit of 767 bytes max key length for InnoDb tables. The max key length limit applies to each column in the (primary key) index. MySql uses up to 3 bytes per character when utf8 character set is used. Therefore, the max length of the file_name column is 255 characgters when using utf8 character set. This change sets the file_name column length to 255 when using mysql. The whole "CREATE TABLE ..." statement is extracted into the MysqlAccountPatchReviewStore subclass. This gives more freedom for further mysql specific tuning. Change-Id: If35aff6c8dc515228c770729bfbf3a83e3dcc6bf --- Documentation/pgm-MigrateAccountPatchReviewDb.txt | 6 ++++++ .../server/schema/JdbcAccountPatchReviewStore.java | 2 +- .../schema/MysqlAccountPatchReviewStore.java | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/Documentation/pgm-MigrateAccountPatchReviewDb.txt b/Documentation/pgm-MigrateAccountPatchReviewDb.txt index 5718a8a5e1..c8ab19380c 100644 --- a/Documentation/pgm-MigrateAccountPatchReviewDb.txt +++ b/Documentation/pgm-MigrateAccountPatchReviewDb.txt @@ -27,6 +27,12 @@ To migrate AccountPatchReviewDb: * Migrate data using this command * Start Gerrit +[NOTE] +When using MySQL, the file_name column length in the account_patch_reviews table will be shortened +from the standard 4096 characters down to 255 characters. This is due to a +link:https://dev.mysql.com/doc/refman/5.7/en/innodb-restrictions.html[MySQL limitation] +on the max size of 767 bytes for each column in an index. + == OPTIONS -d:: diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java index c274e56900..43f39b29d7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/JdbcAccountPatchReviewStore.java @@ -181,7 +181,7 @@ public abstract class JdbcAccountPatchReviewStore } } - private static void doCreateTable(Statement stmt) throws SQLException { + protected void doCreateTable(Statement stmt) throws SQLException { stmt.executeUpdate( "CREATE TABLE IF NOT EXISTS account_patch_reviews (" + "account_id INTEGER DEFAULT 0 NOT NULL, " diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/MysqlAccountPatchReviewStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/MysqlAccountPatchReviewStore.java index af844656e6..d648ed0672 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/MysqlAccountPatchReviewStore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/MysqlAccountPatchReviewStore.java @@ -22,6 +22,7 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; import java.sql.SQLException; +import java.sql.Statement; import org.eclipse.jgit.lib.Config; @Singleton @@ -50,4 +51,17 @@ public class MysqlAccountPatchReviewStore extends JdbcAccountPatchReviewStore { return new OrmException(op + " failure on ACCOUNT_PATCH_REVIEWS", err); } } + + @Override + protected void doCreateTable(Statement stmt) throws SQLException { + stmt.executeUpdate( + "CREATE TABLE IF NOT EXISTS account_patch_reviews (" + + "account_id INTEGER DEFAULT 0 NOT NULL, " + + "change_id INTEGER DEFAULT 0 NOT NULL, " + + "patch_set_id INTEGER DEFAULT 0 NOT NULL, " + + "file_name VARCHAR(255) DEFAULT '' NOT NULL, " + + "CONSTRAINT primary_key_account_patch_reviews " + + "PRIMARY KEY (change_id, patch_set_id, account_id, file_name)" + + ")"); + } } From 6582ae4d4a17487a10f07b495d30be843c01a7dc Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 16 Feb 2018 10:29:48 +0900 Subject: [PATCH 09/10] Set version to 2.15-SNAPSHOT This will allow us to build and deploy a 2.15-SNAPSHOT version of the API artifacts containing changes that have been introduced since 2.15-rc2 was released, thus allowing changes such as I44d794025 to work properly. Change-Id: I4d0c0854ca89cf392f57d7291136a5c41f032a87 --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index 1d5e464212..747f5d4646 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15-rc2 + 2.15-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index f3644dd94b..b6556bc956 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15-rc2 + 2.15-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index c9082a2338..f9dc7e46be 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15-rc2 + 2.15-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index a1bd1c6685..daabb46f7d 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15-rc2 + 2.15-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index 3b1098cc3c..f3dec88c41 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15-rc2 + 2.15-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 804803a378..488a83f68c 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15-rc2" +GERRIT_VERSION = "2.15-SNAPSHOT" From cd127535f70aaaeb0b41aa36b48cb024d2438bff Mon Sep 17 00:00:00 2001 From: Paladox Date: Mon, 19 Feb 2018 14:29:24 +0000 Subject: [PATCH 10/10] Fix IndexCollection propagation into plugin guice injectors This fixes a regression caused by: I3c4616d08e. As the consequence index collection instances were not correctly propagated into plugin's Guice injectors. A test is added in singleusergroup plugin to avoid such breakages in future. Bug: Issue 8398 Change-Id: I87e29a798a83b03f9daa2cfe4708d89becae201e --- .../google/gerrit/server/plugins/PluginGuiceEnvironment.java | 3 ++- plugins/singleusergroup | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java index 740e8d3e5b..effd51ae2b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginGuiceEnvironment.java @@ -34,6 +34,7 @@ import com.google.gerrit.extensions.registration.RegistrationHandle; import com.google.gerrit.extensions.registration.ReloadableRegistrationHandle; import com.google.gerrit.extensions.systemstatus.ServerInformation; import com.google.gerrit.extensions.webui.WebUiPlugin; +import com.google.gerrit.index.IndexCollection; import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.server.util.PluginRequestContext; import com.google.gerrit.server.util.RequestContext; @@ -568,7 +569,7 @@ public class PluginGuiceEnvironment { Class type = key.getTypeLiteral().getRawType(); if (LifecycleListener.class.isAssignableFrom(type) // This is needed for secondary index to work from plugin listeners - && !is("com.google.gerrit.server.index.IndexCollection", type)) { + && !IndexCollection.class.isAssignableFrom(type)) { return false; } if (StartPluginListener.class.isAssignableFrom(type)) { diff --git a/plugins/singleusergroup b/plugins/singleusergroup index 73cfc73077..94e9edbba4 160000 --- a/plugins/singleusergroup +++ b/plugins/singleusergroup @@ -1 +1 @@ -Subproject commit 73cfc73077d249a5d92ae3d31f1949b816bd98c3 +Subproject commit 94e9edbba44b5e8c42e7764af924d3ea20835bf2