opendev/gerrit
Code Issues Proposed changes

Merge "Add extension point invoked between NoteDb migration states" into stable-2.15

Browse Source
This commit is contained in:
Dave Borowitz 2017-10-26 10:25:17 +00:00 committed by Gerrit Code Review
commit 9231509fec
4 changed files with 127 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
View File

@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.server.notedb;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB;
@ -24,6 +25,10 @@ import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WIT
import static com.google.gerrit.server.notedb.NotesMigrationState.REVIEW_DB;
import static com.google.gerrit.server.notedb.NotesMigrationState.WRITE;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.easymock.EasyMock.createStrictMock;
import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.verify;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@ -33,6 +38,8 @@ import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@ -45,12 +52,15 @@ import com.google.gerrit.server.notedb.NoteDbChangeState.RefState;
import com.google.gerrit.server.notedb.NotesMigrationState;
import com.google.gerrit.server.notedb.rebuild.MigrationException;
import com.google.gerrit.server.notedb.rebuild.NoteDbMigrator;
import com.google.gerrit.server.notedb.rebuild.NotesMigrationStateListener;
import com.google.gerrit.server.schema.ReviewDbFactory;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gerrit.testutil.NoteDbMode;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
@ -62,6 +72,7 @@ import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.storage.file.FileBasedConfig;
import org.eclipse.jgit.util.FS;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@ -86,8 +97,10 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
@Inject private SitePaths sitePaths;
@Inject private Provider<NoteDbMigrator.Builder> migratorBuilderProvider;
@Inject private Sequences sequences;
@Inject private DynamicSet<NotesMigrationStateListener> listeners;
private FileBasedConfig noteDbConfig;
private List<RegistrationHandle> addedListeners;
@Before
public void setUp() throws Exception {
@ -95,6 +108,15 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
// Unlike in the running server, for tests, we don't stack notedb.config on gerrit.config.
noteDbConfig = new FileBasedConfig(sitePaths.notedb_config.toFile(), FS.detect());
assertNotesMigrationState(REVIEW_DB, false, false);
addedListeners = new ArrayList<>();
}
@After
public void tearDown() throws Exception {
if (addedListeners != null) {
addedListeners.forEach(RegistrationHandle::remove);
addedListeners = null;
}
}
@Test
@ -413,6 +435,50 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
assertNotesMigrationState(NOTE_DB, false, false);
}
@Test
public void notesMigrationStateListener() throws Exception {
NotesMigrationStateListener listener = createStrictMock(NotesMigrationStateListener.class);
listener.preStateChange(REVIEW_DB, WRITE);
expectLastCall();
listener.preStateChange(WRITE, READ_WRITE_NO_SEQUENCE);
expectLastCall();
listener.preStateChange(READ_WRITE_NO_SEQUENCE, READ_WRITE_WITH_SEQUENCE_REVIEW_DB_PRIMARY);
expectLastCall();
listener.preStateChange(
READ_WRITE_WITH_SEQUENCE_REVIEW_DB_PRIMARY, READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY);
listener.preStateChange(READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY, NOTE_DB);
expectLastCall();
replay(listener);
addListener(listener);
createChange();
migrate(b -> b);
assertNotesMigrationState(NOTE_DB, false, false);
verify(listener);
}
@Test
public void notesMigrationStateListenerFails() throws Exception {
NotesMigrationStateListener listener = createStrictMock(NotesMigrationStateListener.class);
listener.preStateChange(REVIEW_DB, WRITE);
expectLastCall();
listener.preStateChange(WRITE, READ_WRITE_NO_SEQUENCE);
IOException listenerException = new IOException("Listener failed");
expectLastCall().andThrow(listenerException);
replay(listener);
addListener(listener);
createChange();
try {
migrate(b -> b);
assert_().fail("expected IOException");
} catch (IOException e) {
assertThat(e).isSameAs(listenerException);
}
assertNotesMigrationState(WRITE, false, false);
verify(listener);
}
private void assertNotesMigrationState(
NotesMigrationState expected, boolean autoMigrate, boolean trialMode) throws Exception {
assertThat(NotesMigrationState.forNotesMigration(notesMigration)).hasValue(expected);
@ -461,4 +527,8 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
assertThat(e).hasMessageThat().contains(expectMessageContains);
}
}
private void addListener(NotesMigrationStateListener listener) {
addedListeners.add(listeners.add(listener));
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java
View File

@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Change.Id;
import com.google.gerrit.reviewdb.client.Project;
@ -24,6 +25,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
import com.google.gerrit.server.notedb.rebuild.NotesMigrationStateListener;
import com.google.inject.TypeLiteral;
import com.google.inject.name.Names;
import org.eclipse.jgit.lib.Config;
@ -54,6 +56,7 @@ public class NoteDbModule extends FactoryModule {
factory(NoteDbUpdateManager.Factory.class);
factory(RobotCommentNotes.Factory.class);
factory(RobotCommentUpdate.Factory.class);
DynamicSet.setOf(binder(), NotesMigrationStateListener.class);
if (!useTestBindings) {
install(ChangeNotesCache.module());

20
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
View File

@ -42,6 +42,7 @@ import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.gerrit.common.FormatUtil;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
@ -125,6 +126,7 @@ public class NoteDbMigrator implements AutoCloseable {
private final WorkQueue workQueue;
private final MutableNotesMigration globalNotesMigration;
private final PrimaryStorageMigrator primaryStorageMigrator;
private final DynamicSet<NotesMigrationStateListener> listeners;
private int threads;
private ImmutableList<Project.NameKey> projects = ImmutableList.of();
@ -148,7 +150,8 @@ public class NoteDbMigrator implements AutoCloseable {
ChangeRebuilder rebuilder,
WorkQueue workQueue,
MutableNotesMigration globalNotesMigration,
PrimaryStorageMigrator primaryStorageMigrator) {
PrimaryStorageMigrator primaryStorageMigrator,
DynamicSet<NotesMigrationStateListener> listeners) {
// Reload gerrit.config/notedb.config on each migrator invocation, in case a previous
// migration in the same process modified the on-disk contents. This ensures the defaults for
// trial/autoMigrate get set correctly below.
@ -163,6 +166,7 @@ public class NoteDbMigrator implements AutoCloseable {
this.workQueue = workQueue;
this.globalNotesMigration = globalNotesMigration;
this.primaryStorageMigrator = primaryStorageMigrator;
this.listeners = listeners;
this.trial = getTrialMode(cfg);
this.autoMigrate = getAutoMigrate(cfg);
}
@ -320,6 +324,7 @@ public class NoteDbMigrator implements AutoCloseable {
rebuilder,
globalNotesMigration,
primaryStorageMigrator,
listeners,
threads > 1
? MoreExecutors.listeningDecorator(workQueue.createQueue(threads, "RebuildChange"))
: MoreExecutors.newDirectExecutorService(),
@ -344,6 +349,7 @@ public class NoteDbMigrator implements AutoCloseable {
private final ChangeRebuilder rebuilder;
private final MutableNotesMigration globalNotesMigration;
private final PrimaryStorageMigrator primaryStorageMigrator;
private final DynamicSet<NotesMigrationStateListener> listeners;
private final ListeningExecutorService executor;
private final ImmutableList<Project.NameKey> projects;
@ -365,6 +371,7 @@ public class NoteDbMigrator implements AutoCloseable {
ChangeRebuilder rebuilder,
MutableNotesMigration globalNotesMigration,
PrimaryStorageMigrator primaryStorageMigrator,
DynamicSet<NotesMigrationStateListener> listeners,
ListeningExecutorService executor,
ImmutableList<Project.NameKey> projects,
ImmutableList<Change.Id> changes,
@ -390,6 +397,7 @@ public class NoteDbMigrator implements AutoCloseable {
this.userFactory = userFactory;
this.globalNotesMigration = globalNotesMigration;
this.primaryStorageMigrator = primaryStorageMigrator;
this.listeners = listeners;
this.executor = executor;
this.projects = projects;
this.changes = changes;
@ -614,6 +622,9 @@ public class NoteDbMigrator implements AutoCloseable {
? "But found this state:\n" + actualOldState.get().toText()
: "But could not parse the current state"));
}
preStateChange(expectedOldState, newState);
newState.setConfigValues(noteDbConfig);
additionalUpdates.accept(noteDbConfig);
noteDbConfig.save();
@ -625,6 +636,13 @@ public class NoteDbMigrator implements AutoCloseable {
}
}
private void preStateChange(NotesMigrationState oldState, NotesMigrationState newState)
throws IOException {
for (NotesMigrationStateListener listener : listeners) {
listener.preStateChange(oldState, newState);
}
}
private void setControlFlags() throws MigrationException {
synchronized (globalNotesMigration) {
try {

35
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NotesMigrationStateListener.java Normal file
View File

@ -0,0 +1,35 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.notedb.rebuild;
import com.google.gerrit.extensions.annotations.ExtensionPoint;
import com.google.gerrit.server.notedb.NotesMigrationState;
import java.io.IOException;
/** Listener for state changes performed by {@link OnlineNoteDbMigrator}. */
@ExtensionPoint
public interface NotesMigrationStateListener {
/**
* Invoked just before saving the new migration state.
*
* @param oldState state prior to this state change.
* @param newState state after this state change.
* @throws IOException if an error occurred, which will cause the migration to abort. Exceptions
* that should be considered non-fatal must be caught (and ideally logged) by the
* implementation rather than thrown.
*/
void preStateChange(NotesMigrationState oldState, NotesMigrationState newState)
throws IOException;
}