From 528b4d257b4f017ef003e92cba0b2fa53278304a Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 30 Oct 2017 16:20:26 -0400 Subject: LockFailureException: Expose ref names Change-Id: I0bc3c077fe8e0a05e5b36987c959193058ce363e --- .../account/externalids/ExternalIdsUpdate.java | 2 +- .../gerrit/server/git/LockFailureException.java | 27 +++++++++++++++++++++- .../gerrit/server/git/VersionedMetaData.java | 3 ++- .../google/gerrit/server/update/RefUpdateUtil.java | 2 +- 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java index 00dc05a5f1..db37147fc5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java @@ -840,7 +840,7 @@ public class ExternalIdsUpdate { case FORCED: break; case LOCK_FAILURE: - throw new LockFailureException("Updating external IDs failed with " + res); + throw new LockFailureException("Updating external IDs failed with " + res, u); case IO_FAILURE: case NOT_ATTEMPTED: case REJECTED: diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LockFailureException.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LockFailureException.java index 7380b0a686..02a30e098b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LockFailureException.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LockFailureException.java @@ -14,13 +14,38 @@ package com.google.gerrit.server.git; +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.common.collect.ImmutableList; import java.io.IOException; +import org.eclipse.jgit.lib.BatchRefUpdate; +import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.transport.ReceiveCommand; /** Thrown when updating a ref in Git fails with LOCK_FAILURE. */ public class LockFailureException extends IOException { private static final long serialVersionUID = 1L; - public LockFailureException(String message) { + private final ImmutableList refs; + + public LockFailureException(String message, RefUpdate refUpdate) { super(message); + refs = ImmutableList.of(refUpdate.getName()); + } + + public LockFailureException(String message, BatchRefUpdate batchRefUpdate) { + super(message); + refs = + batchRefUpdate + .getCommands() + .stream() + .filter(c -> c.getResult() == ReceiveCommand.Result.LOCK_FAILURE) + .map(ReceiveCommand::getRefName) + .collect(toImmutableList()); + } + + /** Subset of ref names that caused the lock failure. */ + public ImmutableList getFailedRefs() { + return refs; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java index 1bf35864b0..92edc471c8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java @@ -395,7 +395,8 @@ public abstract class VersionedMetaData { + " in " + db.getDirectory() + ": " - + ru.getResult()); + + ru.getResult(), + ru); case FORCED: case IO_FAILURE: case NOT_ATTEMPTED: diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RefUpdateUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RefUpdateUtil.java index ab0b78e2d5..86b4eef7c9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/RefUpdateUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RefUpdateUtil.java @@ -74,7 +74,7 @@ public class RefUpdateUtil { } if (lockFailure + aborted == bru.getCommands().size()) { - throw new LockFailureException("Update aborted with one or more lock failures: " + bru); + throw new LockFailureException("Update aborted with one or more lock failures: " + bru, bru); } else if (failure > 0) { throw new IOException("Update failed: " + bru); } -- cgit v1.2.3 From 73007307c4bbb0a287e2e20a17fd9dadd39019d9 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 30 Oct 2017 15:53:33 -0400 Subject: NoteDbMigrator: Use one NoteDbUpdateManager per project We want to avoid writing out thousands of loose objects during migration by using a custom ObjectInserter implementation. This wouldn't do any good if NoteDbMigrator created a new NoteDbUpdateManager with a new ObjectInserter for each change. Hoist manager creation into NoteDbMigrator instead, even though this requires repeating a bit of logic that previously lived in ChangeRebuilderImpl. The old implementation of NoteDbUpdateManager unconditionally buffered in memory all objects that were inserted, so it could expose them in the StagedResult used by the auto-rebuilding and dry-run codepaths. This behavior is reasonable when modifying a small number of changes, but would not be appropriate for a batch migration where the manager is used to update thousands of changes in a repository. Fortunately, the InsertedObjects aren't needed in the migration codepath, so we can just add a flag to NoteDbUpdateManager to turn this behavior off. Similarly, plumb a flag to NoteDbUpdateManager to turn off atomic ref updates. In an online migration, we fully expect some changes to be modified concurrently, which is already handled by later auto-rebuilding. No need to block a single giant BatchRefUpdate on such changes. Change-Id: I32446fd17d6a2b0cc8b30bd1164402580d7e0aa2 --- .../google/gerrit/server/git/InMemoryInserter.java | 4 + .../google/gerrit/server/notedb/ChangeNotes.java | 1 + .../gerrit/server/notedb/NoteDbUpdateManager.java | 98 +++++++++++++-- .../server/notedb/rebuild/ChangeRebuilderImpl.java | 16 ++- .../server/notedb/rebuild/NoteDbMigrator.java | 131 +++++++++++++++------ 5 files changed, 201 insertions(+), 49 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/InMemoryInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/InMemoryInserter.java index 9c43bdbebc..b80f84669c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/InMemoryInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/InMemoryInserter.java @@ -95,6 +95,10 @@ public class InMemoryInserter extends ObjectInserter { return ImmutableList.copyOf(inserted.values()); } + public int getInsertedObjectCount() { + return inserted.values().size(); + } + public void clear() { inserted.clear(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index c9a28f4ce6..7799d2d6b7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -779,6 +779,7 @@ public class ChangeNotes extends AbstractChangeNotes { rebuildResult = checkNotNull(r); checkNotNull(r.newState()); checkNotNull(r.staged()); + checkNotNull(r.staged().changeObjects()); return LoadHandle.create( ChangeNotesCommit.newStagedRevWalk(repo, r.staged().changeObjects()), r.newState().getChangeMetaId()); 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 ea1f891376..3f20766004 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 @@ -98,13 +98,13 @@ public class NoteDbUpdateManager implements AutoCloseable { ImmutableList changeObjects = ImmutableList.of(); if (changeRepo != null) { changeCommands = changeRepo.getCommandsSnapshot(); - changeObjects = changeRepo.tempIns.getInsertedObjects(); + changeObjects = changeRepo.getInsertedObjects(); } ImmutableList allUsersCommands = ImmutableList.of(); ImmutableList allUsersObjects = ImmutableList.of(); if (allUsersRepo != null) { allUsersCommands = allUsersRepo.getCommandsSnapshot(); - allUsersObjects = allUsersRepo.tempIns.getInsertedObjects(); + allUsersObjects = allUsersRepo.getInsertedObjects(); } return new AutoValue_NoteDbUpdateManager_StagedResult( id, delta, @@ -119,10 +119,32 @@ public class NoteDbUpdateManager implements AutoCloseable { public abstract ImmutableList changeCommands(); + /** + * Objects inserted into the change repo for this change. + * + *

Includes all objects inserted for any change in this repo that may have been processed by + * the corresponding {@link NoteDbUpdateManager} instance, not just those objects that were + * inserted to handle this specific change's updates. + * + * @return inserted objects, or null if the corresponding {@link NoteDbUpdateManager} was + * configured not to {@link NoteDbUpdateManager#setSaveObjects(boolean) save objects}. + */ + @Nullable public abstract ImmutableList changeObjects(); public abstract ImmutableList allUsersCommands(); + /** + * Objects inserted into the All-Users repo for this change. + * + *

Includes all objects inserted into All-Users for any change that may have been processed + * by the corresponding {@link NoteDbUpdateManager} instance, not just those objects that were + * inserted to handle this specific change's updates. + * + * @return inserted objects, or null if the corresponding {@link NoteDbUpdateManager} was + * configured not to {@link NoteDbUpdateManager#setSaveObjects(boolean) save objects}. + */ + @Nullable public abstract ImmutableList allUsersObjects(); } @@ -144,17 +166,20 @@ public class NoteDbUpdateManager implements AutoCloseable { public final RevWalk rw; public final ChainedReceiveCommands cmds; - private final InMemoryInserter tempIns; + private final InMemoryInserter inMemIns; + private final ObjectInserter tempIns; @Nullable private final ObjectInserter finalIns; private final boolean close; + private final boolean saveObjects; private OpenRepo( Repository repo, RevWalk rw, @Nullable ObjectInserter ins, ChainedReceiveCommands cmds, - boolean close) { + boolean close, + boolean saveObjects) { ObjectReader reader = rw.getObjectReader(); checkArgument( ins == null || reader.getCreatedFromInserter() == ins, @@ -162,11 +187,21 @@ public class NoteDbUpdateManager implements AutoCloseable { ins, reader.getCreatedFromInserter()); this.repo = checkNotNull(repo); - this.tempIns = new InMemoryInserter(rw.getObjectReader()); + + if (saveObjects) { + this.inMemIns = new InMemoryInserter(rw.getObjectReader()); + this.tempIns = inMemIns; + } else { + checkArgument(ins != null); + this.inMemIns = null; + this.tempIns = ins; + } + this.rw = new RevWalk(tempIns.newReader()); this.finalIns = ins; this.cmds = checkNotNull(cmds); this.close = close; + this.saveObjects = saveObjects; } public Optional getObjectId(String refName) throws IOException { @@ -177,17 +212,25 @@ public class NoteDbUpdateManager implements AutoCloseable { return ImmutableList.copyOf(cmds.getCommands().values()); } + @Nullable + ImmutableList getInsertedObjects() { + return saveObjects ? inMemIns.getInsertedObjects() : null; + } + void flush() throws IOException { flushToFinalInserter(); finalIns.flush(); } void flushToFinalInserter() throws IOException { + if (!saveObjects) { + return; + } checkState(finalIns != null); - for (InsertedObject obj : tempIns.getInsertedObjects()) { + for (InsertedObject obj : inMemIns.getInsertedObjects()) { finalIns.insert(obj.type(), obj.data().toByteArray()); } - tempIns.clear(); + inMemIns.clear(); } @Override @@ -219,6 +262,8 @@ public class NoteDbUpdateManager implements AutoCloseable { private OpenRepo allUsersRepo; private Map staged; private boolean checkExpectedState = true; + private boolean saveObjects = true; + private boolean atomicRefUpdates = true; private String refLogMessage; private PersonIdent refLogIdent; private PushCertificate pushCert; @@ -264,14 +309,14 @@ public class NoteDbUpdateManager implements AutoCloseable { public NoteDbUpdateManager setChangeRepo( Repository repo, RevWalk rw, @Nullable ObjectInserter ins, ChainedReceiveCommands cmds) { checkState(changeRepo == null, "change repo already initialized"); - changeRepo = new OpenRepo(repo, rw, ins, cmds, false); + changeRepo = new OpenRepo(repo, rw, ins, cmds, false, true); return this; } public NoteDbUpdateManager setAllUsersRepo( Repository repo, RevWalk rw, @Nullable ObjectInserter ins, ChainedReceiveCommands cmds) { checkState(allUsersRepo == null, "All-Users repo already initialized"); - allUsersRepo = new OpenRepo(repo, rw, ins, cmds, false); + allUsersRepo = new OpenRepo(repo, rw, ins, cmds, false, true); return this; } @@ -280,6 +325,37 @@ public class NoteDbUpdateManager implements AutoCloseable { return this; } + /** + * Set whether to save objects and make them available in {@link StagedResult}s. + * + *

If set, all objects inserted into all repos managed by this instance will be buffered in + * memory, and the {@link StagedResult}s will return non-null lists from {@link + * StagedResult#changeObjects()} and {@link StagedResult#allUsersObjects()}. + * + *

Not recommended if modifying a large number of changes with a single manager. + * + * @param saveObjects whether to save objects; defaults to true. + * @return this + */ + public NoteDbUpdateManager setSaveObjects(boolean saveObjects) { + this.saveObjects = saveObjects; + return this; + } + + /** + * Set whether to use atomic ref updates. + * + *

Can be set to false when the change updates represented by this manager aren't logically + * related, e.g. when the updater is only used to group objects together with a single inserter. + * + * @param atomicRefUpdates whether to use atomic ref updates; defaults to true. + * @return this + */ + public NoteDbUpdateManager setAtomicRefUpdates(boolean atomicRefUpdates) { + this.atomicRefUpdates = atomicRefUpdates; + return this; + } + public NoteDbUpdateManager setRefLogMessage(String message) { this.refLogMessage = message; return this; @@ -336,7 +412,7 @@ public class NoteDbUpdateManager implements AutoCloseable { ObjectInserter ins = repo.newObjectInserter(); // Closed by OpenRepo#close. ObjectReader reader = ins.newReader(); // Not closed by OpenRepo#close. try (RevWalk rw = new RevWalk(reader)) { // Doesn't escape OpenRepo constructor. - return new OpenRepo(repo, rw, ins, new ChainedReceiveCommands(repo), true) { + return new OpenRepo(repo, rw, ins, new ChainedReceiveCommands(repo), true, saveObjects) { @Override public void close() { reader.close(); @@ -543,6 +619,7 @@ 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(); } @@ -554,6 +631,7 @@ public class NoteDbUpdateManager implements AutoCloseable { bru.setRefLogMessage(firstNonNull(guessRestApiHandler(), "Update NoteDb refs"), false); } bru.setRefLogIdent(refLogIdent != null ? refLogIdent : serverIdent.get()); + bru.setAtomic(atomicRefUpdates); or.cmds.addTo(bru); bru.setAllowNonFastForwards(true); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java index 166d8a9c53..a28d9c5683 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java @@ -187,7 +187,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } try (NoteDbUpdateManager manager = updateManagerFactory.create(change.getProject())) { buildUpdates(manager, bundleReader.fromReviewDb(db, changeId)); - return execute(db, changeId, manager, checkReadOnly); + return execute(db, changeId, manager, checkReadOnly, true); } } @@ -216,11 +216,15 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { @Override public Result execute(ReviewDb db, Change.Id changeId, NoteDbUpdateManager manager) throws OrmException, IOException { - return execute(db, changeId, manager, true); + return execute(db, changeId, manager, true, true); } public Result execute( - ReviewDb db, Change.Id changeId, NoteDbUpdateManager manager, boolean checkReadOnly) + ReviewDb db, + Change.Id changeId, + NoteDbUpdateManager manager, + boolean checkReadOnly, + boolean executeManager) throws OrmException, IOException { db = ReviewDbUtil.unwrapDb(db); Change change = checkNoteDbState(ChangeNotes.readOneReviewDbChange(db, changeId)); @@ -277,11 +281,13 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { // to the caller so they know to use the staged results instead of reading from the repo. throw new OrmException(NoteDbUpdateManager.CHANGES_READ_ONLY); } - manager.execute(); + if (executeManager) { + manager.execute(); + } return r; } - private static Change checkNoteDbState(Change c) throws OrmException { + static Change checkNoteDbState(Change c) throws OrmException { // Can only rebuild a change if its primary storage is ReviewDb. NoteDbChangeState s = NoteDbChangeState.parse(c); if (s != null && s.getPrimaryStorage() != PrimaryStorage.REVIEW_DB) { 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 add6d57a3b..3f72ee5fb2 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 @@ -26,6 +26,7 @@ import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WIT import static com.google.gerrit.server.notedb.NotesMigrationState.WRITE; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Comparator.comparing; +import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import com.google.common.annotations.VisibleForTesting; @@ -56,12 +57,16 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.LockFailureException; import com.google.gerrit.server.git.WorkQueue; +import com.google.gerrit.server.notedb.ChangeBundleReader; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.MutableNotesMigration; import com.google.gerrit.server.notedb.NoteDbTable; +import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.NotesMigrationState; import com.google.gerrit.server.notedb.PrimaryStorageMigrator; 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.util.ManualRequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.gwtorm.server.OrmException; @@ -74,8 +79,10 @@ 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; @@ -119,10 +126,12 @@ public class NoteDbMigrator implements AutoCloseable { private final SitePaths sitePaths; private final SchemaFactory schemaFactory; private final GitRepositoryManager repoManager; + private final NoteDbUpdateManager.Factory updateManagerFactory; + private final ChangeBundleReader bundleReader; private final AllProjectsName allProjects; private final InternalUser.Factory userFactory; private final ThreadLocalRequestContext requestContext; - private final ChangeRebuilder rebuilder; + private final ChangeRebuilderImpl rebuilder; private final WorkQueue workQueue; private final MutableNotesMigration globalNotesMigration; private final PrimaryStorageMigrator primaryStorageMigrator; @@ -144,10 +153,12 @@ public class NoteDbMigrator implements AutoCloseable { SitePaths sitePaths, SchemaFactory schemaFactory, GitRepositoryManager repoManager, + NoteDbUpdateManager.Factory updateManagerFactory, + ChangeBundleReader bundleReader, AllProjectsName allProjects, ThreadLocalRequestContext requestContext, InternalUser.Factory userFactory, - ChangeRebuilder rebuilder, + ChangeRebuilderImpl rebuilder, WorkQueue workQueue, MutableNotesMigration globalNotesMigration, PrimaryStorageMigrator primaryStorageMigrator, @@ -159,6 +170,8 @@ public class NoteDbMigrator implements AutoCloseable { this.sitePaths = sitePaths; this.schemaFactory = schemaFactory; this.repoManager = repoManager; + this.updateManagerFactory = updateManagerFactory; + this.bundleReader = bundleReader; this.allProjects = allProjects; this.requestContext = requestContext; this.userFactory = userFactory; @@ -318,6 +331,8 @@ public class NoteDbMigrator implements AutoCloseable { sitePaths, schemaFactory, repoManager, + updateManagerFactory, + bundleReader, allProjects, requestContext, userFactory, @@ -343,10 +358,12 @@ public class NoteDbMigrator implements AutoCloseable { private final FileBasedConfig noteDbConfig; private final SchemaFactory schemaFactory; private final GitRepositoryManager repoManager; + private final NoteDbUpdateManager.Factory updateManagerFactory; + private final ChangeBundleReader bundleReader; private final AllProjectsName allProjects; private final ThreadLocalRequestContext requestContext; private final InternalUser.Factory userFactory; - private final ChangeRebuilder rebuilder; + private final ChangeRebuilderImpl rebuilder; private final MutableNotesMigration globalNotesMigration; private final PrimaryStorageMigrator primaryStorageMigrator; private final DynamicSet listeners; @@ -365,10 +382,12 @@ public class NoteDbMigrator implements AutoCloseable { SitePaths sitePaths, SchemaFactory schemaFactory, GitRepositoryManager repoManager, + NoteDbUpdateManager.Factory updateManagerFactory, + ChangeBundleReader bundleReader, AllProjectsName allProjects, ThreadLocalRequestContext requestContext, InternalUser.Factory userFactory, - ChangeRebuilder rebuilder, + ChangeRebuilderImpl rebuilder, MutableNotesMigration globalNotesMigration, PrimaryStorageMigrator primaryStorageMigrator, DynamicSet listeners, @@ -392,6 +411,8 @@ public class NoteDbMigrator implements AutoCloseable { this.schemaFactory = schemaFactory; this.rebuilder = rebuilder; this.repoManager = repoManager; + this.updateManagerFactory = updateManagerFactory; + this.bundleReader = bundleReader; this.allProjects = allProjects; this.requestContext = requestContext; this.userFactory = userFactory; @@ -729,43 +750,85 @@ public class NoteDbMigrator implements AutoCloseable { ProgressMonitor pm = new TextProgressMonitor( new PrintWriter(new BufferedWriter(new OutputStreamWriter(progressOut, UTF_8)))); - pm.beginTask(FormatUtil.elide(project.get(), 50), allChanges.get(project).size()); - try { + try (NoteDbUpdateManager manager = + updateManagerFactory.create(project).setSaveObjects(false).setAtomicRefUpdates(false)) { + Set skipExecute = new HashSet<>(); Collection changes = allChanges.get(project); - for (Change.Id changeId : changes) { - // Update one change at a time, which ends up creating one NoteDbUpdateManager per change as - // well. This turns out to be no more expensive than batching, since each NoteDb operation - // is only writing single loose ref updates and loose objects. Plus we have to do one - // ReviewDb transaction per change due to the AtomicUpdate, so if we somehow batched NoteDb - // operations, ReviewDb would become the bottleneck. - try { - rebuilder.rebuild(db, changeId); - } catch (NoPatchSetsException e) { - log.warn(e.getMessage()); - } catch (RepositoryNotFoundException e) { - log.warn("Repository {} not found while rebuilding change {}", project, changeId); - } catch (ConflictingUpdateException e) { - log.warn( - "Rebuilding detected a conflicting ReviewDb update for change {};" - + " will be auto-rebuilt at runtime", - changeId); - } catch (LockFailureException e) { - log.warn( - "Rebuilding detected a conflicting NoteDb update for change {};" - + " will be auto-rebuilt at runtime", - changeId); - } catch (Throwable t) { - log.error("Failed to rebuild change " + changeId, t); - ok = false; + pm.beginTask(FormatUtil.elide("Rebuilding " + project.get(), 50), changes.size()); + try { + for (Change.Id changeId : changes) { + boolean staged = false; + try { + stage(db, changeId, manager); + staged = true; + } catch (NoPatchSetsException e) { + log.warn(e.getMessage()); + } catch (RepositoryNotFoundException e) { + log.warn("Repository {} not found while rebuilding change {}", project, changeId); + } 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) { + log.warn( + "Rebuilding detected a conflicting ReviewDb update for change {};" + + " will be auto-rebuilt at runtime", + changeId); + } catch (Throwable t) { + log.error("Failed to rebuild change " + changeId, t); + ok = false; + } + pm.update(1); } - pm.update(1); + } finally { + pm.endTask(); + } + + try { + manager.execute(); + } 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) { + log.error("Failed to save NoteDb state for " + project, e); } - } finally { - pm.endTask(); } return ok; } + private void stage(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. + Change change = + ChangeRebuilderImpl.checkNoteDbState(ChangeNotes.readOneReviewDbChange(db, changeId)); + if (change == null) { + // Could log here instead, but this matches the behavior of ChangeRebuilderImpl#stage. + throw new NoSuchChangeException(changeId); + } + rebuilder.buildUpdates(manager, bundleReader.fromReviewDb(db, changeId)); + } + private static boolean futuresToBoolean(List> futures, String errMsg) { try { return Futures.allAsList(futures).get().stream().allMatch(b -> b); -- cgit v1.2.3 From e1cfc28d3449fc9180a5be9ac8a0b45d65550124 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 1 Nov 2017 12:52:51 -0400 Subject: Upgrade JGit to 4.9.0.201710071750-r.8-g678c99c05 Includes a new ObjectInserter implementation for use with NoteDb. Still based off the stable-4.9 branch. Bug: Issue 7399 Change-Id: Ifa305e7e9eb1ff9092b3ad62174640702b2ebe04 --- lib/jgit/jgit.bzl | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/jgit/jgit.bzl b/lib/jgit/jgit.bzl index 1d964b8291..27f53c7ec5 100644 --- a/lib/jgit/jgit.bzl +++ b/lib/jgit/jgit.bzl @@ -1,12 +1,12 @@ load("//tools/bzl:maven_jar.bzl", "GERRIT", "MAVEN_LOCAL", "MAVEN_CENTRAL", "maven_jar") -_JGIT_VERS = "4.9.0.201710071750-r" +_JGIT_VERS = "4.9.0.201710071750-r.8-g678c99c05" -_DOC_VERS = _JGIT_VERS # Set to _JGIT_VERS unless using a snapshot +_DOC_VERS = "4.9.0.201710071750-r" # Set to _JGIT_VERS unless using a snapshot JGIT_DOC_URL = "http://download.eclipse.org/jgit/site/" + _DOC_VERS + "/apidocs" -_JGIT_REPO = MAVEN_CENTRAL # Leave here even if set to MAVEN_CENTRAL. +_JGIT_REPO = GERRIT # Leave here even if set to MAVEN_CENTRAL. # set this to use a local version. # "/home//projects/jgit" @@ -26,28 +26,28 @@ def jgit_maven_repos(): name = "jgit_lib", artifact = "org.eclipse.jgit:org.eclipse.jgit:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "69d8510b335d4d33d551a133505a4141311f970a", - src_sha1 = "6fd1eb331447b6163898b4d10aa769e2ac193740", + sha1 = "845fcaabaaf84f0924f78971def3adc962d3a69e", + src_sha1 = "8cc0627939d1bcd8fb313fd6bed03edfecc5011f", unsign = True, ) maven_jar( name = "jgit_servlet", artifact = "org.eclipse.jgit:org.eclipse.jgit.http.server:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "93fb0075988b9c6bb97c725c03706f2341965b6b", + sha1 = "a0cb65ef0d9db387b010337b44ac4710be52b21e", unsign = True, ) maven_jar( name = "jgit_archive", artifact = "org.eclipse.jgit:org.eclipse.jgit.archive:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "a15aee805c758516ad7e9fa3f16e27bb9f4a1c2e", + sha1 = "32c85ea349d19a0129d43ceda021a3bdd41a8242", ) maven_jar( name = "jgit_junit", artifact = "org.eclipse.jgit:org.eclipse.jgit.junit:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "b6e712e743ea5798134f54547ae80456fad07f76", + sha1 = "549c3b338d66c2f8575394252a39e69d7fe71969", unsign = True, ) -- cgit v1.2.3 From 155a251740502e0f2e6f7ba804517659018cb01a Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 31 Oct 2017 07:54:11 -0400 Subject: NoteDbMigrator: Use new pack-based inserter implementation The migrator can potentially create many thousands of new objects for NoteDb data, more than we want to leave loose in the repository. Upstream JGit added a new ObjectInserter implementation that inserts objects directly into packs instead. We can now use this to share a single inserter for the whole change repository. Only use this strategy during migration. We would rather make loose objects in a live server, since object lookup is linear in the number of packs, so we wouldn't want to continually create new packs without compacting them. Bug: Issue 7399 Change-Id: I3963875525e7963f209081a6881c4c77e015122e --- .../server/notedb/OnlineNoteDbMigrationIT.java | 34 +++++++++++++++++- .../gerrit/server/notedb/NoteDbUpdateManager.java | 4 +-- .../server/notedb/rebuild/NoteDbMigrator.java | 40 +++++++++++++++++++--- 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java index 51e2964d81..6b142635ca 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java @@ -60,8 +60,17 @@ import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import com.google.inject.Provider; import java.io.IOException; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; import java.util.List; +import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; +import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; @@ -374,10 +383,14 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest { Change.Id id1 = r1.getChange().getId(); Change.Id id2 = r2.getChange().getId(); + Set objectFiles = getObjectFiles(project); + assertThat(objectFiles).isNotEmpty(); + migrate(b -> b.setThreads(threads)); - assertNotesMigrationState(NOTE_DB, false, false); + assertNotesMigrationState(NOTE_DB, false, false); assertThat(sequences.nextChangeId()).isEqualTo(503); + assertThat(getObjectFiles(project)).containsExactlyElementsIn(objectFiles); ObjectId oldMetaId = null; int rowVersion = 0; @@ -531,4 +544,23 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest { private void addListener(NotesMigrationStateListener listener) { addedListeners.add(listeners.add(listener)); } + + private SortedSet getObjectFiles(Project.NameKey project) throws Exception { + SortedSet files = new TreeSet<>(); + try (Repository repo = repoManager.openRepository(project)) { + Files.walkFileTree( + ((FileRepository) repo).getObjectDatabase().getDirectory().toPath(), + new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) { + String name = file.getFileName().toString(); + if (!attrs.isDirectory() && !name.endsWith(".pack") && !name.endsWith(".idx")) { + files.add(name); + } + return FileVisitResult.CONTINUE; + } + }); + } + return files; + } } 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 3f20766004..3e36de93bb 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 @@ -309,14 +309,14 @@ public class NoteDbUpdateManager implements AutoCloseable { public NoteDbUpdateManager setChangeRepo( Repository repo, RevWalk rw, @Nullable ObjectInserter ins, ChainedReceiveCommands cmds) { checkState(changeRepo == null, "change repo already initialized"); - changeRepo = new OpenRepo(repo, rw, ins, cmds, false, true); + changeRepo = new OpenRepo(repo, rw, ins, cmds, false, saveObjects); return this; } public NoteDbUpdateManager setAllUsersRepo( Repository repo, RevWalk rw, @Nullable ObjectInserter ins, ChainedReceiveCommands cmds) { checkState(allUsersRepo == null, "All-Users repo already initialized"); - allUsersRepo = new OpenRepo(repo, rw, ins, cmds, false, true); + allUsersRepo = new OpenRepo(repo, rw, ins, cmds, false, saveObjects); return this; } 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 3f72ee5fb2..29811745ae 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 @@ -67,6 +67,7 @@ import com.google.gerrit.server.notedb.PrimaryStorageMigrator; 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.util.ManualRequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.gwtorm.server.OrmException; @@ -89,9 +90,14 @@ import java.util.function.Consumer; import java.util.function.Predicate; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.ObjectReader; 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.util.FS; import org.eclipse.jgit.util.io.NullOutputStream; @@ -741,6 +747,12 @@ public class NoteDbMigrator implements AutoCloseable { return ImmutableListMultimap.copyOf(out); } + private static ObjectInserter newPackInserter(Repository repo) { + return repo instanceof FileRepository + ? ((FileRepository) repo).getObjectDatabase().newPackInserter() + : repo.newObjectInserter(); + } + private boolean rebuildProject( ReviewDb db, ImmutableListMultimap allChanges, @@ -750,8 +762,26 @@ public class NoteDbMigrator implements AutoCloseable { ProgressMonitor pm = new TextProgressMonitor( new PrintWriter(new BufferedWriter(new OutputStreamWriter(progressOut, UTF_8)))); - try (NoteDbUpdateManager manager = - updateManagerFactory.create(project).setSaveObjects(false).setAtomicRefUpdates(false)) { + try (Repository changeRepo = repoManager.openRepository(project); + 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<>(); Collection changes = allChanges.get(project); pm.beginTask(FormatUtil.elide("Rebuilding " + project.get(), 50), changes.size()); @@ -763,8 +793,6 @@ public class NoteDbMigrator implements AutoCloseable { staged = true; } catch (NoPatchSetsException e) { log.warn(e.getMessage()); - } catch (RepositoryNotFoundException e) { - log.warn("Repository {} not found while rebuilding change {}", project, changeId); } catch (Throwable t) { log.error("Failed to rebuild change " + changeId, t); ok = false; @@ -812,6 +840,10 @@ public class NoteDbMigrator implements AutoCloseable { } catch (OrmException | IOException e) { log.error("Failed to save NoteDb state for " + project, e); } + } catch (RepositoryNotFoundException e) { + log.warn("Repository {} not found", project); + } catch (IOException e) { + log.error("Failed to rebuild project " + project, e); } return ok; } -- cgit v1.2.3