opendev/gerrit
Code Issues Proposed changes

Don't fuse code and meta ref updates if it won't be atomic

Browse Source 
Prior to fusing updates in Ia5b34bae, BatchUpdate ensured that code
refs updated in updateRepo were all updated before meta refs in
updateChange. Fusing them is safe when BatchRefUpdate is an atomic
transaction in the underlying storage, but it's actually a regression
when the storage doesn't support atomic multi-ref operations. This is
because all updates in the batch can fail independently, so we can end
up with a meta ref update succeeding but the corresponding code update
failing.

To handle this case, temporarily resurrect the old NoteDbBatchUpdate
implementation from 3915d7baa, switching between the fused/unfused
implementations based on a new config option. It's an error to try to
execute a FusedNoteDbBatchUpdate on a repository that doesn't support
atomic transactions, but due to the way BatchUpdate.Factory works, we
have to decide which type of update to instantiate before we have
opened any repos. So we need to have a separate config option to signal
this; add a new NoteDbMode FUSED to run tests in this case.

To keep our tests realistic, use the atomic feature of
InMemoryRepository only if we are running in FUSED mode. Setting this
reveals a latent issue where we need to setAtomic(false), so fix that
as well.

Ideally this workaround of having multiple backends is a temporary
measure until RefDirectory gains the ability to perform multi-ref
transactions[1]. Long term we would like to be able to eliminate this.
This somewhat depends, however, on if there are other RefDatabase
implementations in the wild that people are likely to use with NoteDb
that don't support atomic transactions.

[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=515678

Change-Id: I8e67dc88c7ca7d59ef3157cfb6194571a0521828
This commit is contained in:
Dave Borowitz
2017-04-24 13:42:19 +02:00
parent ecd4831c73
commit c7078ab07c
12 changed files with 596 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@@ -896,7 +896,14 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
PushResult pr =
GitUtil.pushHead(testRepo, "refs/for/foo%base=" + rBase.getCommit().name(), false, false);
assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
if (notesMigration.fuseUpdates()) {
// InMemoryRepository's atomic BatchRefUpdate implementation doesn't update the progress
// monitor. That's fine, we just care that there was at least one new change and no errors.
assertThat(pr.getMessages()).contains("changes: new: 1, done");
} else {
assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
}
assertTwoChangesWithSameRevision(r);
}

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java
View File

@@ -31,7 +31,6 @@ import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.testutil.NoteDbMode;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.EnumSet;
@@ -48,11 +47,12 @@ public class NoteDbOnlyIT extends AbstractDaemonTest {
@Before
public void setUp() throws Exception {
assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.DISABLE_CHANGE_REVIEW_DB);
assume().that(notesMigration.disableChangeReviewDb()).isTrue();
}
@Test
public void updateChangeFailureRollsBackRefUpdate() throws Exception {
assume().that(notesMigration.fuseUpdates()).isTrue();
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();