opendev/gerrit
Code Issues Proposed changes

Remove config option to read account sequence numbers from ReviewDb

Browse Source 
Accounts have been migrated to NoteDb, hence also the account sequence
should be moved to NoteDb.

Change-Id: I7da683e61b6ef0368efaec17b1f4562cf6a050ac
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-07-10 09:51:17 +02:00
parent 5be9c31001
commit c05fba2505
6 changed files with 31 additions and 213 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
Documentation/config-gerrit.txt
View File

@@ -3285,17 +3285,6 @@ NoteDb is the next generation of Gerrit storage backend, currently powering
`googlesource.com`. It is not (yet) recommended for general use, but if you want
to learn more, see the link:dev-note-db.html[developer documentation].
[[notedb.accounts.readSequenceFromNoteDb]]notedb.accounts.readSequenceFromNoteDb::
+
Whether the account sequence should be read from NoteDb.
+
Once set to `true` this parameter cannot be set back to `false` because
the account sequence in ReviewDb will no longer be updated when account
IDs are retrieved from NoteDb, and hence the account sequence in
ReviewDb will be outdated.
+
By default `false`.
[[notedb.accounts.sequenceBatchSize]]notedb.accounts.sequenceBatchSize::
+
The current account sequence number is stored as UTF-8 text in a blob
@@ -3307,9 +3296,6 @@ hand out numbers from that range in memory until they run out. This
configuration parameter controls the size of the account ID batch that
each process retrieves at once.
+
Only relevant if link:#notedb.accounts.readSequenceFromNoteDb[
notedb.accounts.readSequenceFromNoteDb] is set to `true`.
+
By default, 1.
[[oauth]]

17
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AccountCreator.java
View File

@@ -32,11 +32,8 @@ import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
import com.google.gerrit.server.ssh.SshKeyCache;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import com.jcraft.jsch.JSch;
import com.jcraft.jsch.JSchException;
@@ -54,8 +51,7 @@ public class AccountCreator {
private final Map<String, TestAccount> accounts;
private final SchemaFactory<ReviewDb> reviewDbProvider;
private final OneOffRequestContext oneOffRequestContext;
private final Provider<Sequences> sequencesProvider;
private final Sequences sequences;
private final AccountsUpdate.Server accountsUpdate;
private final VersionedAuthorizedKeys.Accessor authorizedKeys;
private final GroupCache groupCache;
@@ -68,8 +64,7 @@ public class AccountCreator {
@Inject
AccountCreator(
SchemaFactory<ReviewDb> schema,
OneOffRequestContext oneOffRequestContext,
Provider<Sequences> sequencesProvider,
Sequences sequences,
AccountsUpdate.Server accountsUpdate,
VersionedAuthorizedKeys.Accessor authorizedKeys,
GroupCache groupCache,
@@ -80,8 +75,7 @@ public class AccountCreator {
@SshEnabled boolean sshEnabled) {
accounts = new HashMap<>();
reviewDbProvider = schema;
this.oneOffRequestContext = oneOffRequestContext;
this.sequencesProvider = sequencesProvider;
this.sequences = sequences;
this.accountsUpdate = accountsUpdate;
this.authorizedKeys = authorizedKeys;
this.groupCache = groupCache;
@@ -103,9 +97,8 @@ public class AccountCreator {
if (account != null) {
return account;
}
try (ReviewDb db = reviewDbProvider.open();
ManualRequestContext ctx = oneOffRequestContext.open()) {
Account.Id id = new Account.Id(sequencesProvider.get().nextAccountId());
try (ReviewDb db = reviewDbProvider.open()) {
Account.Id id = new Account.Id(sequences.nextAccountId());
List<ExternalId> extIds = new ArrayList<>(2);
String httpPass = null;

23
gerrit-server/src/main/java/com/google/gerrit/server/Sequences.java
View File

@@ -54,7 +54,6 @@ public class Sequences {
private final Provider<ReviewDb> db;
private final NotesMigration migration;
private final boolean readAccountSeqFromNoteDb;
private final RepoSequence accountSeq;
private final RepoSequence changeSeq;
private final Timer2<SequenceType, Boolean> nextIdLatency;
@@ -71,18 +70,14 @@ public class Sequences {
this.db = db;
this.migration = migration;
readAccountSeqFromNoteDb =
cfg.getBoolean("noteDb", "accounts", "readSequenceFromNoteDb", false);
// It's intentional to not use any configured gap for the account sequence unlike we do for the
// change sequence. For the account sequence we have a different migration strategy that
// doesn't require any gap.
@SuppressWarnings("deprecation")
RepoSequence.Seed accountSeed = () -> db.get().nextAccountId();
int accountBatchSize = cfg.getInt("noteDb", "accounts", "sequenceBatchSize", 1);
accountSeq =
new RepoSequence(repoManager, allUsers, NAME_ACCOUNTS, accountSeed, accountBatchSize);
new RepoSequence(
repoManager,
allUsers,
NAME_ACCOUNTS,
() -> ReviewDb.FIRST_ACCOUNT_ID,
accountBatchSize);
int gap = getChangeSequenceGap(cfg);
@SuppressWarnings("deprecation")
@@ -102,17 +97,11 @@ public class Sequences {
}
public int nextAccountId() throws OrmException {
if (readAccountSeqFromNoteDb) {
try (Timer2.Context timer = nextIdLatency.start(SequenceType.ACCOUNTS, false)) {
return accountSeq.next();
}
}
int accountId = nextAccountId(db.get());
accountSeq.increaseTo(accountId + 1); // NoteDb stores next available account ID.
return accountId;
}
public int nextChangeId() throws OrmException {
if (!migration.readChangeSequence()) {
return nextChangeId(db.get());

16
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
View File

@@ -34,8 +34,6 @@ import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.account.InternalAccountQuery;
import com.google.gerrit.server.util.ManualRequestContext;
import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
@@ -60,8 +58,7 @@ public class AccountManager {
private static final Logger log = LoggerFactory.getLogger(AccountManager.class);
private final SchemaFactory<ReviewDb> schema;
private final OneOffRequestContext oneOffRequestContext;
private final Provider<Sequences> sequencesProvider;
private final Sequences sequences;
private final Accounts accounts;
private final AccountsUpdate.Server accountsUpdateFactory;
private final AccountCache byIdCache;
@@ -79,8 +76,7 @@ public class AccountManager {
@Inject
AccountManager(
SchemaFactory<ReviewDb> schema,
OneOffRequestContext oneOffRequestContext,
Provider<Sequences> sequencesProvider,
Sequences sequences,
@GerritServerConfig Config cfg,
Accounts accounts,
AccountsUpdate.Server accountsUpdateFactory,
@@ -95,8 +91,7 @@ public class AccountManager {
ExternalIds externalIds,
ExternalIdsUpdate.Server externalIdsUpdateFactory) {
this.schema = schema;
this.oneOffRequestContext = oneOffRequestContext;
this.sequencesProvider = sequencesProvider;
this.sequences = sequences;
this.accounts = accounts;
this.accountsUpdateFactory = accountsUpdateFactory;
this.byIdCache = byIdCache;
@@ -214,10 +209,7 @@ public class AccountManager {
private AuthResult create(ReviewDb db, AuthRequest who)
throws OrmException, AccountException, IOException, ConfigInvalidException {
Account.Id newId;
try (ManualRequestContext ctx = oneOffRequestContext.open()) {
newId = new Account.Id(sequencesProvider.get().nextAccountId());
}
Account.Id newId = new Account.Id(sequences.nextAccountId());
ExternalId extId =
ExternalId.createWithEmail(who.getExternalIdKey(), newId, who.getEmailAddress());

61
gerrit-server/src/main/java/com/google/gerrit/server/notedb/RepoSequence.java
View File

@@ -194,27 +194,6 @@ public class RepoSequence {
}
}
public void increaseTo(int val) throws OrmException {
counterLock.lock();
try {
try (Repository repo = repoManager.openRepository(projectName);
RevWalk rw = new RevWalk(repo)) {
TryIncreaseTo attempt = new TryIncreaseTo(repo, rw, val);
checkResult(retryer.call(attempt));
counter = limit;
} catch (ExecutionException | RetryException e) {
if (e.getCause() != null) {
Throwables.throwIfInstanceOf(e.getCause(), OrmException.class);
}
throw new OrmException(e);
} catch (IOException e) {
throw new OrmException(e);
}
} finally {
counterLock.unlock();
}
}
private void acquire(int count) throws OrmException {
try (Repository repo = repoManager.openRepository(projectName);
RevWalk rw = new RevWalk(repo)) {
@@ -234,9 +213,7 @@ public class RepoSequence {
}
private void checkResult(RefUpdate.Result result) throws OrmException {
if (result != RefUpdate.Result.NEW
&& result != RefUpdate.Result.FORCED
&& result != RefUpdate.Result.NO_CHANGE) {
if (result != RefUpdate.Result.NEW && result != RefUpdate.Result.FORCED) {
throw new OrmException("failed to update " + refName + ": " + result);
}
}
@@ -264,43 +241,12 @@ public class RepoSequence {
next = seed.get();
} else {
oldId = ref.getObjectId();
next = parse(rw, oldId);
next = parse(oldId);
}
return store(repo, rw, oldId, next + count);
}
}
private class TryIncreaseTo implements Callable<RefUpdate.Result> {
private final Repository repo;
private final RevWalk rw;
private final int value;
private TryIncreaseTo(Repository repo, RevWalk rw, int value) {
this.repo = repo;
this.rw = rw;
this.value = value;
}
@Override
public RefUpdate.Result call() throws Exception {
Ref ref = repo.exactRef(refName);
afterReadRef.run();
ObjectId oldId;
if (ref == null) {
oldId = ObjectId.zeroId();
} else {
oldId = ref.getObjectId();
int next = parse(rw, oldId);
if (next >= value) {
// a concurrent write updated the ref already to this or a higher value
return RefUpdate.Result.NO_CHANGE;
}
}
return store(repo, rw, oldId, value);
}
}
private int parse(RevWalk rw, ObjectId id) throws IOException, OrmException {
private int parse(ObjectId id) throws IOException, OrmException {
ObjectLoader ol = rw.getObjectReader().open(id, OBJ_BLOB);
if (ol.getType() != OBJ_BLOB) {
// In theory this should be thrown by open but not all implementations
@@ -314,6 +260,7 @@ public class RepoSequence {
}
return val;
}
}
private RefUpdate.Result store(Repository repo, RevWalk rw, @Nullable ObjectId oldId, int val)
throws IOException {

89
gerrit-server/src/test/java/com/google/gerrit/server/notedb/RepoSequenceTest.java
View File

@@ -253,95 +253,6 @@ public class RepoSequenceTest {
assertThat(s2.acquireCount).isEqualTo(1);
}
@Test
public void increaseTo() throws Exception {
// Seed existing ref value.
writeBlob("id", "1");
RepoSequence s = newSequence("id", 1, 10);
s.increaseTo(2);
assertThat(s.next()).isEqualTo(2);
}
@Test
public void increaseToLowerValueIsIgnored() throws Exception {
// Seed existing ref value.
writeBlob("id", "2");
RepoSequence s = newSequence("id", 1, 10);
s.increaseTo(1);
assertThat(s.next()).isEqualTo(2);
}
@Test
public void increaseToRetryOnLockFailureV1() throws Exception {
// Seed existing ref value.
writeBlob("id", "1");
AtomicBoolean doneBgUpdate = new AtomicBoolean(false);
Runnable bgUpdate =
() -> {
if (!doneBgUpdate.getAndSet(true)) {
writeBlob("id", "2");
}
};
RepoSequence s = newSequence("id", 1, 10, bgUpdate, RETRYER);
assertThat(doneBgUpdate.get()).isFalse();
// Increase the value to 3. The background thread increases the value to 2, which makes the
// increase to value 3 fail once with LockFailure. The increase to 3 is then retried and is
// expected to succeed.
s.increaseTo(3);
assertThat(s.next()).isEqualTo(3);
assertThat(doneBgUpdate.get()).isTrue();
}
@Test
public void increaseToRetryOnLockFailureV2() throws Exception {
// Seed existing ref value.
writeBlob("id", "1");
AtomicBoolean doneBgUpdate = new AtomicBoolean(false);
Runnable bgUpdate =
() -> {
if (!doneBgUpdate.getAndSet(true)) {
writeBlob("id", "3");
}
};
RepoSequence s = newSequence("id", 1, 10, bgUpdate, RETRYER);
assertThat(doneBgUpdate.get()).isFalse();
// Increase the value to 2. The background thread increases the value to 3, which makes the
// increase to value 2 fail with LockFailure. The increase to 2 is then not retried because the
// current value is already higher and it should be preserved.
s.increaseTo(2);
assertThat(s.next()).isEqualTo(3);
assertThat(doneBgUpdate.get()).isTrue();
}
@Test
public void increaseToFailAfterRetryerGivesUp() throws Exception {
AtomicInteger bgCounter = new AtomicInteger(1234);
RepoSequence s =
newSequence(
"id",
1,
10,
() -> writeBlob("id", Integer.toString(bgCounter.getAndAdd(1000))),
RetryerBuilder.<RefUpdate.Result>newBuilder()
.withStopStrategy(StopStrategies.stopAfterAttempt(3))
.build());
exception.expect(OrmException.class);
exception.expectMessage("failed to update refs/sequences/id: LOCK_FAILURE");
s.increaseTo(2);
}
private RepoSequence newSequence(String name, int start, int batchSize) {
return newSequence(name, start, batchSize, Runnables.doNothing(), RETRYER);
}