opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.15'

Browse Source 
Prefer the JGit version from master, which is based off of JGit master
and contains the same commits as in the stable branch.

* stable-2.15:
  SetReadyForReview: Change action label to "Start Review"
  Tighten vertical spacing in messages
  NoteDbMigrator: Use new pack-based inserter implementation
  Upgrade JGit to 4.9.0.201710071750-r.8-g678c99c05
  NoteDbMigrator: Use one NoteDbUpdateManager per project
  LockFailureException: Expose ref names
  ChangeRebuilderImpl: Handle createdOn > lastUpdatedOn

Change-Id: Ic842d494fff7641de24ebd15ca33e4e91a5c400d
This commit is contained in:
Dave Borowitz
2017-11-02 08:40:33 -04:00
parent 7ea0eda1da 8843019833
commit a08e56114c
14 changed files with 498 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/google/gerrit/server/account/externalids/ExternalIdsUpdate.java
View File

@@ -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:

2
java/com/google/gerrit/server/change/SetReadyForReview.java
View File

@@ -75,7 +75,7 @@ public class SetReadyForReview extends RetryingRestModifyView<ChangeResource, In
@Override
public Description getDescription(ChangeResource rsrc) {
return new Description()
.setLabel("Ready")
.setLabel("Start Review")
.setTitle("Set Ready For Review")
.setVisible(
rsrc.isUserOwner()

4
java/com/google/gerrit/server/git/InMemoryInserter.java
View File

@@ -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();
}

27
java/com/google/gerrit/server/git/LockFailureException.java
View File

@@ -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<String> 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<String> getFailedRefs() {
return refs;
}
}

3
java/com/google/gerrit/server/git/VersionedMetaData.java
View File

@@ -395,7 +395,8 @@ public abstract class VersionedMetaData {
+ " in "
+ db.getDirectory()
+ ": "
+ ru.getResult());
+ ru.getResult(),
ru);
case FORCED:
case IO_FAILURE:
case NOT_ATTEMPTED:

41
java/com/google/gerrit/server/notedb/ChangeBundle.java
View File

@@ -68,6 +68,7 @@ import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
@@ -387,6 +388,8 @@ public class ChangeBundle {
boolean excludeCreatedOn = false;
boolean excludeCurrentPatchSetId = false;
boolean excludeTopic = false;
Timestamp aCreated = a.getCreatedOn();
Timestamp bCreated = b.getCreatedOn();
Timestamp aUpdated = a.getLastUpdatedOn();
Timestamp bUpdated = b.getLastUpdatedOn();
@@ -397,8 +400,10 @@ public class ChangeBundle {
String aSubj = Strings.nullToEmpty(a.getSubject());
String bSubj = Strings.nullToEmpty(b.getSubject());
// Allow created timestamp in NoteDb to be either the created timestamp of
// the change, or the timestamp of the first remaining patch set.
// Allow created timestamp in NoteDb to be any of:
// - The created timestamp of the change.
// - The timestamp of the first remaining patch set.
// - The last updated timestamp, if it is less than the created timestamp.
//
// Ignore subject if the NoteDb subject starts with the ReviewDb subject.
// The NoteDb subject is read directly from the commit, whereas the ReviewDb
@@ -434,8 +439,14 @@ public class ChangeBundle {
//
// Use max timestamp of all ReviewDb entities when comparing with NoteDb.
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
boolean createdOnMatchesFirstPs =
!timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, bCreated);
boolean createdOnMatchesLastUpdatedOn =
!timestampsDiffer(bundleA, aUpdated, bundleB, bCreated);
boolean createdAfterUpdated = aCreated.compareTo(aUpdated) > 0;
excludeCreatedOn =
!timestampsDiffer(bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
createdOnMatchesFirstPs || (createdAfterUpdated && createdOnMatchesLastUpdatedOn);
aSubj = cleanReviewDbSubject(aSubj);
bSubj = cleanNoteDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleA.validPatchSetPredicate().apply(a.currentPatchSetId());
@@ -446,8 +457,14 @@ public class ChangeBundle {
Objects.equals(aTopic, b.getTopic()) || "".equals(aTopic) && b.getTopic() == null;
aUpdated = bundleA.getLatestTimestamp();
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
boolean createdOnMatchesFirstPs =
!timestampsDiffer(bundleA, aCreated, bundleB, bundleB.getFirstPatchSetTime());
boolean createdOnMatchesLastUpdatedOn =
!timestampsDiffer(bundleA, aCreated, bundleB, bUpdated);
boolean createdAfterUpdated = bCreated.compareTo(bUpdated) > 0;
excludeCreatedOn =
!timestampsDiffer(bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
createdOnMatchesFirstPs || (createdAfterUpdated && createdOnMatchesLastUpdatedOn);
aSubj = cleanNoteDbSubject(aSubj);
bSubj = cleanReviewDbSubject(bSubj);
excludeCurrentPatchSetId = !bundleB.validPatchSetPredicate().apply(b.currentPatchSetId());
@@ -651,6 +668,8 @@ public class ChangeBundle {
List<String> diffs, ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchSet.Id, PatchSet> as = bundleA.patchSets;
Map<PatchSet.Id, PatchSet> bs = bundleB.patchSets;
Optional<PatchSet.Id> minA = as.keySet().stream().min(intKeyOrdering());
Optional<PatchSet.Id> minB = bs.keySet().stream().min(intKeyOrdering());
Set<PatchSet.Id> ids = diffKeySets(diffs, as, bs);
// Old versions of Gerrit had a bug that created patch sets during
@@ -663,11 +682,14 @@ public class ChangeBundle {
// ignore the createdOn timestamps if both:
// * ReviewDb timestamps are non-monotonic.
// * NoteDb timestamps are monotonic.
boolean excludeCreatedOn = false;
//
// Allow the timestamp of the first patch set to match the creation time of
// the change.
boolean excludeAllCreatedOn = false;
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
excludeCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
excludeAllCreatedOn = !createdOnIsMonotonic(as, ids) && createdOnIsMonotonic(bs, ids);
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
excludeAllCreatedOn = createdOnIsMonotonic(as, ids) && !createdOnIsMonotonic(bs, ids);
}
for (PatchSet.Id id : ids) {
@@ -676,11 +698,16 @@ public class ChangeBundle {
String desc = describe(id);
String pushCertField = "pushCertificate";
boolean excludeCreatedOn = excludeAllCreatedOn;
boolean excludeDesc = false;
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
excludeDesc = Objects.equals(trimOrNull(a.getDescription()), b.getDescription());
excludeCreatedOn |=
Optional.of(id).equals(minB) && b.getCreatedOn().equals(bundleB.change.getCreatedOn());
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeDesc = Objects.equals(a.getDescription(), trimOrNull(b.getDescription()));
excludeCreatedOn |=
Optional.of(id).equals(minA) && a.getCreatedOn().equals(bundleA.change.getCreatedOn());
}
List<String> exclude = Lists.newArrayList(pushCertField);

1
java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -779,6 +779,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
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());

98
java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
View File

@@ -98,13 +98,13 @@ public class NoteDbUpdateManager implements AutoCloseable {
ImmutableList<InsertedObject> changeObjects = ImmutableList.of();
if (changeRepo != null) {
changeCommands = changeRepo.getCommandsSnapshot();
changeObjects = changeRepo.tempIns.getInsertedObjects();
changeObjects = changeRepo.getInsertedObjects();
}
ImmutableList<ReceiveCommand> allUsersCommands = ImmutableList.of();
ImmutableList<InsertedObject> 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<ReceiveCommand> changeCommands();
/**
* Objects inserted into the change repo for this change.
*
* <p>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<InsertedObject> changeObjects();
public abstract ImmutableList<ReceiveCommand> allUsersCommands();
/**
* Objects inserted into the All-Users repo for this change.
*
* <p>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<InsertedObject> 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<ObjectId> getObjectId(String refName) throws IOException {
@@ -177,17 +212,25 @@ public class NoteDbUpdateManager implements AutoCloseable {
return ImmutableList.copyOf(cmds.getCommands().values());
}
@Nullable
ImmutableList<InsertedObject> 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<Change.Id, StagedResult> 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, 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);
allUsersRepo = new OpenRepo(repo, rw, ins, cmds, false, saveObjects);
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.
*
* <p>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()}.
*
* <p>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.
*
* <p>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);

25
java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
View File

@@ -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);
}
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) {
@@ -299,6 +305,13 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
if (bundle.getPatchSets().isEmpty()) {
throw new NoPatchSetsException(change.getId());
}
if (change.getLastUpdatedOn().compareTo(change.getCreatedOn()) < 0) {
// A bug in data migration might set created_on to the time of the migration. The
// correct timestamps were lost, but we can at least set it so created_on is not after
// last_updated_on.
// See https://bugs.chromium.org/p/gerrit/issues/detail?id=7397
change.setCreatedOn(change.getLastUpdatedOn());
}
// We will rebuild all events, except for draft comments, in buckets based on author and
// timestamp.
@@ -428,9 +441,11 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
new EventSorter(events).sort();
// Ensure the first event in the list creates the change, setting the author and any required
// footers.
// footers. Also force the creation time of the first patch set to match the creation time of
// the change.
Event first = events.get(0);
if (first instanceof PatchSetEvent && change.getOwner().equals(first.user)) {
first.when = change.getCreatedOn();
((PatchSetEvent) first).createChange = true;
} else {
events.add(0, new CreateChangeEvent(change, minPsNum));

135
java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
View File

@@ -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,17 @@ 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.update.ChainedReceiveCommands;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.gwtorm.server.OrmException;
@@ -74,17 +80,24 @@ 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;
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;
@@ -119,10 +132,12 @@ public class NoteDbMigrator implements AutoCloseable {
private final SitePaths sitePaths;
private final SchemaFactory<ReviewDb> 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 +159,12 @@ public class NoteDbMigrator implements AutoCloseable {
SitePaths sitePaths,
SchemaFactory<ReviewDb> 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 +176,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 +337,8 @@ public class NoteDbMigrator implements AutoCloseable {
sitePaths,
schemaFactory,
repoManager,
updateManagerFactory,
bundleReader,
allProjects,
requestContext,
userFactory,
@@ -343,10 +364,12 @@ public class NoteDbMigrator implements AutoCloseable {
private final FileBasedConfig noteDbConfig;
private final SchemaFactory<ReviewDb> 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<NotesMigrationStateListener> listeners;
@@ -365,10 +388,12 @@ public class NoteDbMigrator implements AutoCloseable {
SitePaths sitePaths,
SchemaFactory<ReviewDb> schemaFactory,
GitRepositoryManager repoManager,
NoteDbUpdateManager.Factory updateManagerFactory,
ChangeBundleReader bundleReader,
AllProjectsName allProjects,
ThreadLocalRequestContext requestContext,
InternalUser.Factory userFactory,
ChangeRebuilder rebuilder,
ChangeRebuilderImpl rebuilder,
MutableNotesMigration globalNotesMigration,
PrimaryStorageMigrator primaryStorageMigrator,
DynamicSet<NotesMigrationStateListener> listeners,
@@ -392,6 +417,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;
@@ -720,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<Project.NameKey, Change.Id> allChanges,
@@ -729,31 +762,64 @@ 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 (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<Change.Id> skipExecute = new HashSet<>();
Collection<Change.Id> 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.
pm.beginTask(FormatUtil.elide("Rebuilding " + project.get(), 50), changes.size());
try {
rebuilder.rebuild(db, changeId);
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 (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;
@@ -763,9 +829,38 @@ public class NoteDbMigrator implements AutoCloseable {
} 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);
}
} catch (RepositoryNotFoundException e) {
log.warn("Repository {} not found", project);
} catch (IOException e) {
log.error("Failed to rebuild project " + project, e);
}
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<ListenableFuture<Boolean>> futures, String errMsg) {
try {
return Futures.allAsList(futures).get().stream().allMatch(b -> b);

2
java/com/google/gerrit/server/update/RefUpdateUtil.java
View File

@@ -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);
}

39
javatests/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@@ -866,6 +866,45 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
assertThat(notes.getChange().getSubject()).isEqualTo(PushOneCommit.SUBJECT);
}
@Test
public void allTimestampsExceptUpdatedAreEqualDueToBadMigration() throws Exception {
// https://bugs.chromium.org/p/gerrit/issues/detail?id=7397
PushOneCommit.Result r = createChange();
Change c = r.getChange().change();
Change.Id id = c.getId();
Timestamp ts = TimeUtil.nowTs();
Timestamp origUpdated = c.getLastUpdatedOn();
c.setCreatedOn(ts);
assertThat(c.getCreatedOn()).isGreaterThan(c.getLastUpdatedOn());
db.changes().update(Collections.singleton(c));
List<ChangeMessage> cm = db.changeMessages().byChange(id).toList();
cm.forEach(m -> m.setWrittenOn(ts));
db.changeMessages().update(cm);
List<PatchSet> ps = db.patchSets().byChange(id).toList();
ps.forEach(p -> p.setCreatedOn(ts));
db.patchSets().update(ps);
List<PatchSetApproval> psa = db.patchSetApprovals().byChange(id).toList();
psa.forEach(p -> p.setGranted(ts));
db.patchSetApprovals().update(psa);
List<PatchLineComment> plc = db.patchComments().byChange(id).toList();
plc.forEach(p -> p.setWrittenOn(ts));
db.patchComments().update(plc);
checker.rebuildAndCheckChanges(id);
setNotesMigration(true, true);
ChangeNotes notes = notesFactory.create(db, project, id);
assertThat(notes.getChange().getCreatedOn()).isEqualTo(origUpdated);
assertThat(notes.getChange().getLastUpdatedOn()).isAtLeast(origUpdated);
assertThat(notes.getPatchSets().get(new PatchSet.Id(id, 1)).getCreatedOn())
.isEqualTo(origUpdated);
}
@Test
public void createWithAutoRebuildingDisabled() throws Exception {
ReviewDb oldDb = db;

36
javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
View File

@@ -59,8 +59,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;
@@ -373,10 +382,14 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
Change.Id id1 = r1.getChange().getId();
Change.Id id2 = r2.getChange().getId();
migrate(b -> b.setThreads(threads));
assertNotesMigrationState(NOTE_DB, false, false);
Set<String> objectFiles = getObjectFiles(project);
assertThat(objectFiles).isNotEmpty();
migrate(b -> b.setThreads(threads));
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<String> getObjectFiles(Project.NameKey project) throws Exception {
SortedSet<String> files = new TreeSet<>();
try (Repository repo = repoManager.openRepository(project)) {
Files.walkFileTree(
((FileRepository) repo).getObjectDatabase().getDirectory().toPath(),
new SimpleFileVisitor<Path>() {
@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;
}
}

117
javatests/com/google/gerrit/server/notedb/ChangeBundleTest.java
View File

@@ -667,6 +667,39 @@ public class ChangeBundleTest extends GerritBaseTests {
assertNoDiffs(b2, b1);
}
@Test
public void diffChangesAllowsCreatedToMatchLastUpdated() throws Exception {
Change c1 = TestChanges.newChange(new Project.NameKey("project"), new Account.Id(100));
c1.setCreatedOn(TimeUtil.nowTs());
assertThat(c1.getCreatedOn()).isGreaterThan(c1.getLastUpdatedOn());
Change c2 = clone(c1);
c2.setCreatedOn(c2.getLastUpdatedOn());
// Both ReviewDb.
ChangeBundle b1 =
new ChangeBundle(
c1, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 =
new ChangeBundle(
c2, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
assertDiffs(
b1,
b2,
"createdOn differs for Change.Id "
+ c1.getId()
+ ": {2009-09-30 17:00:06.0} != {2009-09-30 17:00:00.0}");
// One NoteDb.
b1 =
new ChangeBundle(
c1, messages(), patchSets(), approvals(), comments(), reviewers(), REVIEW_DB);
b2 =
new ChangeBundle(
c2, messages(), patchSets(), approvals(), comments(), reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
}
@Test
public void diffChangeMessageKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);
@@ -1393,6 +1426,90 @@ public class ChangeBundleTest extends GerritBaseTests {
+ " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:18.0}");
}
@Test
public void diffPatchSetsAllowsFirstPatchSetCreatedOnToMatchChangeCreatedOn() {
Change c = TestChanges.newChange(project, accountId);
c.setLastUpdatedOn(TimeUtil.nowTs());
PatchSet goodPs1 = new PatchSet(new PatchSet.Id(c.getId(), 1));
goodPs1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
goodPs1.setUploader(accountId);
goodPs1.setCreatedOn(TimeUtil.nowTs());
assertThat(goodPs1.getCreatedOn()).isGreaterThan(c.getCreatedOn());
PatchSet ps1AtCreatedOn = clone(goodPs1);
ps1AtCreatedOn.setCreatedOn(c.getCreatedOn());
PatchSet goodPs2 = new PatchSet(new PatchSet.Id(c.getId(), 2));
goodPs2.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
goodPs2.setUploader(accountId);
goodPs2.setCreatedOn(TimeUtil.nowTs());
PatchSet ps2AtCreatedOn = clone(goodPs2);
ps2AtCreatedOn.setCreatedOn(c.getCreatedOn());
// Both ReviewDb, exact match required.
ChangeBundle b1 =
new ChangeBundle(
c,
messages(),
patchSets(goodPs1, goodPs2),
approvals(),
comments(),
reviewers(),
REVIEW_DB);
ChangeBundle b2 =
new ChangeBundle(
c,
messages(),
patchSets(ps1AtCreatedOn, ps2AtCreatedOn),
approvals(),
comments(),
reviewers(),
REVIEW_DB);
assertDiffs(
b1,
b2,
"createdOn differs for PatchSet.Id "
+ c.getId()
+ ",1: {2009-09-30 17:00:12.0} != {2009-09-30 17:00:00.0}",
"createdOn differs for PatchSet.Id "
+ c.getId()
+ ",2: {2009-09-30 17:00:18.0} != {2009-09-30 17:00:00.0}");
// One ReviewDb, PS1 is allowed to match change createdOn, but PS2 isn't.
b1 =
new ChangeBundle(
c,
messages(),
patchSets(goodPs1, goodPs2),
approvals(),
comments(),
reviewers(),
REVIEW_DB);
b2 =
new ChangeBundle(
c,
messages(),
patchSets(ps1AtCreatedOn, ps2AtCreatedOn),
approvals(),
comments(),
reviewers(),
NOTE_DB);
assertDiffs(
b1,
b2,
"createdOn differs for PatchSet.Id "
+ c.getId()
+ ",2 in NoteDb vs. ReviewDb: {2009-09-30 17:00:00.0} != {2009-09-30 17:00:18.0}");
assertDiffs(
b2,
b1,
"createdOn differs for PatchSet.Id "
+ c.getId()
+ ",2 in NoteDb vs. ReviewDb: {2009-09-30 17:00:00.0} != {2009-09-30 17:00:18.0}");
}
@Test
public void diffPatchSetApprovalKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);