Use BatchRefUpdate to update accounts
In future we want to update accounts and external IDs atomically. Since
the account properties and external IDs are stored in different refs
(user branch and refs/meta/external-ids) we need to use a BatchRefUpdate
to update the refs atomically. At the moment we still only update the
user branch, but this change is a preparation for being able to update
refs/meta/external-ids branch in the same transaction.
When we start to do updates in multiple branches we also should now
always use the server identity as committer. The calling user is still
set as author if the User factory is used.
With this change we now also open the All-Users repository only once for
reading and writing.
Change-Id: I134bb9ba7f73733a006bd6a5d39486a042f2dc4a
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java
index 730bb95..2e10af4 100644
--- a/java/com/google/gerrit/server/account/AccountsUpdate.java
+++ b/java/com/google/gerrit/server/account/AccountsUpdate.java
@@ -31,6 +31,7 @@
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.index.change.ReindexAfterRefUpdate;
import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
+import com.google.gerrit.server.update.RefUpdateUtil;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gwtorm.server.OrmDuplicateKeyException;
import com.google.gwtorm.server.OrmException;
@@ -42,6 +43,7 @@
import java.util.Optional;
import java.util.function.Consumer;
import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
@@ -111,8 +113,8 @@
private final GitReferenceUpdated gitRefUpdated;
private final AllUsersName allUsersName;
private final OutgoingEmailValidator emailValidator;
- private final Provider<PersonIdent> serverIdent;
- private final Provider<MetaDataUpdate.Server> metaDataUpdateServerFactory;
+ private final Provider<PersonIdent> serverIdentProvider;
+ private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
private final RetryHelper retryHelper;
@Inject
@@ -121,29 +123,30 @@
GitReferenceUpdated gitRefUpdated,
AllUsersName allUsersName,
OutgoingEmailValidator emailValidator,
- @GerritPersonIdent Provider<PersonIdent> serverIdent,
- Provider<MetaDataUpdate.Server> metaDataUpdateServerFactory,
+ @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
RetryHelper retryHelper) {
this.repoManager = repoManager;
this.gitRefUpdated = gitRefUpdated;
this.allUsersName = allUsersName;
this.emailValidator = emailValidator;
- this.serverIdent = serverIdent;
- this.metaDataUpdateServerFactory = metaDataUpdateServerFactory;
+ this.serverIdentProvider = serverIdentProvider;
+ this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
this.retryHelper = retryHelper;
}
public AccountsUpdate create() {
- PersonIdent i = serverIdent.get();
+ PersonIdent serverIdent = serverIdentProvider.get();
return new AccountsUpdate(
repoManager,
gitRefUpdated,
null,
allUsersName,
emailValidator,
- i,
- () -> metaDataUpdateServerFactory.get().create(allUsersName),
- retryHelper);
+ metaDataUpdateInternalFactory,
+ retryHelper,
+ serverIdent,
+ serverIdent);
}
}
@@ -159,9 +162,9 @@
private final GitReferenceUpdated gitRefUpdated;
private final AllUsersName allUsersName;
private final OutgoingEmailValidator emailValidator;
- private final Provider<PersonIdent> serverIdent;
+ private final Provider<PersonIdent> serverIdentProvider;
private final Provider<IdentifiedUser> identifiedUser;
- private final Provider<MetaDataUpdate.User> metaDataUpdateUserFactory;
+ private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
private final RetryHelper retryHelper;
@Inject
@@ -170,32 +173,34 @@
GitReferenceUpdated gitRefUpdated,
AllUsersName allUsersName,
OutgoingEmailValidator emailValidator,
- @GerritPersonIdent Provider<PersonIdent> serverIdent,
+ @GerritPersonIdent Provider<PersonIdent> serverIdentProvider,
Provider<IdentifiedUser> identifiedUser,
- Provider<MetaDataUpdate.User> metaDataUpdateUserFactory,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
RetryHelper retryHelper) {
this.repoManager = repoManager;
this.gitRefUpdated = gitRefUpdated;
this.allUsersName = allUsersName;
- this.serverIdent = serverIdent;
+ this.serverIdentProvider = serverIdentProvider;
this.emailValidator = emailValidator;
this.identifiedUser = identifiedUser;
- this.metaDataUpdateUserFactory = metaDataUpdateUserFactory;
+ this.metaDataUpdateInternalFactory = metaDataUpdateInternalFactory;
this.retryHelper = retryHelper;
}
public AccountsUpdate create() {
IdentifiedUser user = identifiedUser.get();
- PersonIdent i = serverIdent.get();
+ PersonIdent serverIdent = serverIdentProvider.get();
+ PersonIdent userIdent = createPersonIdent(serverIdent, user);
return new AccountsUpdate(
repoManager,
gitRefUpdated,
user,
allUsersName,
emailValidator,
- createPersonIdent(i, user),
- () -> metaDataUpdateUserFactory.get().create(allUsersName),
- retryHelper);
+ metaDataUpdateInternalFactory,
+ retryHelper,
+ serverIdent,
+ userIdent);
}
private PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) {
@@ -208,9 +213,10 @@
@Nullable private final IdentifiedUser currentUser;
private final AllUsersName allUsersName;
private final OutgoingEmailValidator emailValidator;
- private final PersonIdent committerIdent;
- private final MetaDataUpdateFactory metaDataUpdateFactory;
+ private final Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
private final RetryHelper retryHelper;
+ private final PersonIdent committerIdent;
+ private final PersonIdent authorIdent;
private final Runnable afterReadRevision;
private AccountsUpdate(
@@ -219,18 +225,20 @@
@Nullable IdentifiedUser currentUser,
AllUsersName allUsersName,
OutgoingEmailValidator emailValidator,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
+ RetryHelper retryHelper,
PersonIdent committerIdent,
- MetaDataUpdateFactory metaDataUpdateFactory,
- RetryHelper retryHelper) {
+ PersonIdent authorIdent) {
this(
repoManager,
gitRefUpdated,
currentUser,
allUsersName,
emailValidator,
- committerIdent,
- metaDataUpdateFactory,
+ metaDataUpdateInternalFactory,
retryHelper,
+ committerIdent,
+ authorIdent,
Runnables.doNothing());
}
@@ -241,18 +249,21 @@
@Nullable IdentifiedUser currentUser,
AllUsersName allUsersName,
OutgoingEmailValidator emailValidator,
- PersonIdent committerIdent,
- MetaDataUpdateFactory metaDataUpdateFactory,
+ Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory,
RetryHelper retryHelper,
+ PersonIdent committerIdent,
+ PersonIdent authorIdent,
Runnable afterReadRevision) {
this.repoManager = checkNotNull(repoManager, "repoManager");
this.gitRefUpdated = checkNotNull(gitRefUpdated, "gitRefUpdated");
this.currentUser = currentUser;
this.allUsersName = checkNotNull(allUsersName, "allUsersName");
this.emailValidator = checkNotNull(emailValidator, "emailValidator");
- this.committerIdent = checkNotNull(committerIdent, "committerIdent");
- this.metaDataUpdateFactory = checkNotNull(metaDataUpdateFactory, "metaDataUpdateFactory");
+ this.metaDataUpdateInternalFactory =
+ checkNotNull(metaDataUpdateInternalFactory, "metaDataUpdateInternalFactory");
this.retryHelper = checkNotNull(retryHelper, "retryHelper");
+ this.committerIdent = checkNotNull(committerIdent, "committerIdent");
+ this.authorIdent = checkNotNull(authorIdent, "authorIdent");
this.afterReadRevision = afterReadRevision;
}
@@ -286,8 +297,8 @@
public Account insert(Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException {
return updateAccount(
- () -> {
- AccountConfig accountConfig = read(accountId);
+ r -> {
+ AccountConfig accountConfig = read(r, accountId);
Account account = accountConfig.getNewAccount();
InternalAccountUpdate.Builder updateBuilder = InternalAccountUpdate.builder();
updater.update(account, updateBuilder);
@@ -332,8 +343,8 @@
public Account update(Account.Id accountId, AccountUpdater updater)
throws OrmException, IOException, ConfigInvalidException {
return updateAccount(
- () -> {
- AccountConfig accountConfig = read(accountId);
+ r -> {
+ AccountConfig accountConfig = read(r, accountId);
Optional<Account> account = accountConfig.getLoadedAccount();
if (!account.isPresent()) {
return null;
@@ -383,7 +394,7 @@
private void deleteUserBranch(Account.Id accountId) throws IOException {
try (Repository repo = repoManager.openRepository(allUsersName)) {
- deleteUserBranch(repo, allUsersName, gitRefUpdated, currentUser, committerIdent, accountId);
+ deleteUserBranch(repo, allUsersName, gitRefUpdated, currentUser, authorIdent, accountId);
}
}
@@ -414,66 +425,84 @@
gitRefUpdated.fire(project, ru, user != null ? user.getAccount() : null);
}
- private AccountConfig read(Account.Id accountId) throws IOException, ConfigInvalidException {
- try (Repository repo = repoManager.openRepository(allUsersName)) {
- AccountConfig accountConfig = new AccountConfig(emailValidator, accountId);
- accountConfig.load(repo);
+ private AccountConfig read(Repository allUsersRepo, Account.Id accountId)
+ throws IOException, ConfigInvalidException {
+ AccountConfig accountConfig = new AccountConfig(emailValidator, accountId);
+ accountConfig.load(allUsersRepo);
- afterReadRevision.run();
+ afterReadRevision.run();
- return accountConfig;
- }
+ return accountConfig;
}
private Account updateAccount(AccountUpdate accountUpdate)
throws IOException, ConfigInvalidException, OrmException {
return retryHelper.execute(
() -> {
- UpdatedAccount updatedAccount = accountUpdate.update();
- if (updatedAccount == null) {
- return null;
- }
+ try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
+ UpdatedAccount updatedAccount = accountUpdate.update(allUsersRepo);
+ if (updatedAccount == null) {
+ return null;
+ }
- commit(updatedAccount);
- return updatedAccount.getAccount();
+ commit(allUsersRepo, updatedAccount);
+ return updatedAccount.getAccount();
+ }
});
}
- private void commit(UpdatedAccount updatedAccount) throws IOException {
+ private void commit(Repository allUsersRepo, UpdatedAccount updatedAccount) throws IOException {
+ BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate();
if (updatedAccount.isCreated()) {
- commitNew(updatedAccount.getAccountConfig());
+ commitNewAccountConfig(allUsersRepo, batchRefUpdate, updatedAccount.getAccountConfig());
} else {
- commit(updatedAccount.getAccountConfig());
+ commitAccountConfig(allUsersRepo, batchRefUpdate, updatedAccount.getAccountConfig());
}
+ RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo);
+ gitRefUpdated.fire(
+ allUsersName, batchRefUpdate, currentUser != null ? currentUser.getAccount() : null);
}
- private void commitNew(AccountConfig accountConfig) throws IOException {
+ private void commitNewAccountConfig(
+ Repository allUsersRepo, BatchRefUpdate batchRefUpdate, AccountConfig accountConfig)
+ throws IOException {
// When creating a new account we must allow empty commits so that the user branch gets created
// with an empty commit when no account properties are set and hence no 'account.config' file
// will be created.
- commit(accountConfig, true);
+ commitAccountConfig(allUsersRepo, batchRefUpdate, accountConfig, true);
}
- private void commit(AccountConfig accountConfig) throws IOException {
- commit(accountConfig, false);
+ private void commitAccountConfig(
+ Repository allUsersRepo, BatchRefUpdate batchRefUpdate, AccountConfig accountConfig)
+ throws IOException {
+ commitAccountConfig(allUsersRepo, batchRefUpdate, accountConfig, false);
}
- private void commit(AccountConfig accountConfig, boolean allowEmptyCommit) throws IOException {
- try (MetaDataUpdate md = metaDataUpdateFactory.create()) {
+ private void commitAccountConfig(
+ Repository allUsersRepo,
+ BatchRefUpdate batchRefUpdate,
+ AccountConfig accountConfig,
+ boolean allowEmptyCommit)
+ throws IOException {
+ try (MetaDataUpdate md = createMetaDataUpdate(allUsersRepo, batchRefUpdate)) {
md.setAllowEmpty(allowEmptyCommit);
accountConfig.commit(md);
}
}
- @VisibleForTesting
- @FunctionalInterface
- public static interface MetaDataUpdateFactory {
- MetaDataUpdate create() throws IOException;
+ private MetaDataUpdate createMetaDataUpdate(
+ Repository allUsersRepo, BatchRefUpdate batchRefUpdate) {
+ MetaDataUpdate metaDataUpdate =
+ metaDataUpdateInternalFactory.get().create(allUsersName, allUsersRepo, batchRefUpdate);
+ metaDataUpdate.getCommitBuilder().setCommitter(committerIdent);
+ metaDataUpdate.getCommitBuilder().setAuthor(authorIdent);
+ return metaDataUpdate;
}
@FunctionalInterface
private static interface AccountUpdate {
- UpdatedAccount update() throws IOException, ConfigInvalidException, OrmException;
+ UpdatedAccount update(Repository allUsersRepo)
+ throws IOException, ConfigInvalidException, OrmException;
}
private static class UpdatedAccount {
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index e296747..0d3a831 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -95,6 +95,7 @@
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.LockFailureException;
+import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.index.account.StalenessChecker;
@@ -195,6 +196,8 @@
@Inject private RetryHelper.Metrics retryMetrics;
+ @Inject private Provider<MetaDataUpdate.InternalFactory> metaDataUpdateInternalFactory;
+
@Inject
@Named("accounts")
private LoadingCache<Account.Id, Optional<AccountState>> accountsCache;
@@ -1899,6 +1902,7 @@
String status = "happy";
String fullName = "Foo";
AtomicBoolean doneBgUpdate = new AtomicBoolean(false);
+ PersonIdent ident = serverIdent.get();
AccountsUpdate update =
new AccountsUpdate(
repoManager,
@@ -1906,8 +1910,7 @@
null,
allUsers,
emailValidator,
- serverIdent.get(),
- () -> metaDataUpdateFactory.create(allUsers),
+ metaDataUpdateInternalFactory,
new RetryHelper(
cfg,
retryMetrics,
@@ -1915,6 +1918,8 @@
null,
null,
r -> r.withBlockStrategy(noSleepBlockStrategy)),
+ ident,
+ ident,
() -> {
if (!doneBgUpdate.getAndSet(true)) {
try {
@@ -1945,6 +1950,7 @@
List<String> status = ImmutableList.of("foo", "bar", "baz");
String fullName = "Foo";
AtomicInteger bgCounter = new AtomicInteger(0);
+ PersonIdent ident = serverIdent.get();
AccountsUpdate update =
new AccountsUpdate(
repoManager,
@@ -1952,8 +1958,7 @@
null,
allUsers,
emailValidator,
- serverIdent.get(),
- () -> metaDataUpdateFactory.create(allUsers),
+ metaDataUpdateInternalFactory,
new RetryHelper(
cfg,
retryMetrics,
@@ -1963,6 +1968,8 @@
r ->
r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
.withBlockStrategy(noSleepBlockStrategy)),
+ ident,
+ ident,
() -> {
try {
accountsUpdate
