opendev/gerrit
Code Issues Proposed changes

Let AccountsUpdate take care to evict accounts from the account cache

Browse Source 
Accounts need to be evicted from the account cache when they are
updated. Instead of leaving this responsibility to the caller let
AccountsUpdate take care of this. This is more convenient and ensures
that none of the callers forgets about it.

Accounts also need to be evicted from the account cache on creation,
since the account cache may hold an empty AccountState instance for that
account ID (if it was requested from the account cache before and was
found missing).

After updating accounts they also need to be reindexed, but this is
automatically done when accounts are evicted from the account cache.

Change-Id: I08d5c756eabd43c29bdc217e7b1c3d85ebf15197
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-06-07 09:42:11 +02:00
parent 3ea63124f3
commit f36b3d7a75
14 changed files with 38 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -31,7 +31,6 @@ import com.google.gerrit.server.account.GroupCache;
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.index.account.AccountIndexer;
import com.google.gerrit.server.ssh.SshKeyCache;
import com.google.gerrit.testutil.SshMode;
import com.google.gwtorm.server.SchemaFactory;
@@ -59,7 +58,6 @@ public class AccountCreator {
private final SshKeyCache sshKeyCache;
private final AccountCache accountCache;
private final AccountByEmailCache byEmailCache;
private final AccountIndexer indexer;
private final ExternalIdsUpdate.Server externalIdsUpdate;
@Inject
@@ -71,7 +69,6 @@ public class AccountCreator {
SshKeyCache sshKeyCache,
AccountCache accountCache,
AccountByEmailCache byEmailCache,
AccountIndexer indexer,
ExternalIdsUpdate.Server externalIdsUpdate) {
accounts = new HashMap<>();
reviewDbProvider = schema;
@@ -81,7 +78,6 @@ public class AccountCreator {
this.sshKeyCache = sshKeyCache;
this.accountCache = accountCache;
this.byEmailCache = byEmailCache;
this.indexer = indexer;
this.externalIdsUpdate = externalIdsUpdate;
}
@@ -138,8 +134,6 @@ public class AccountCreator {
}
byEmailCache.evict(email);
indexer.index(id);
account = new TestAccount(id, username, email, fullName, sshKey, httpPass);
if (username != null) {
accounts.put(username, account);

7
gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
View File

@@ -36,7 +36,6 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.AccountsUpdate;
@@ -54,7 +53,6 @@ import com.google.inject.Inject;
import com.google.inject.Injector;
import com.google.inject.Provider;
import com.google.inject.util.Providers;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
@@ -77,8 +75,6 @@ public class GerritPublicKeyCheckerTest {
@Inject private AccountsUpdate.Server accountsUpdate;
@Inject private AccountCache accountCache;
@Inject private AccountManager accountManager;
@Inject private GerritPublicKeyChecker.Factory checkerFactory;
@@ -156,8 +152,7 @@ public class GerritPublicKeyCheckerTest {
return userFactory.create(id);
}
private IdentifiedUser reloadUser() throws IOException {
accountCache.evict(userId);
private IdentifiedUser reloadUser() {
user = userFactory.create(userId);
return user;
}

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

@@ -200,9 +200,6 @@ public class AccountManager {
byEmailCache.evict(oldEmail);
byEmailCache.evict(newEmail);
}
if (toUpdate != null) {
byIdCache.evict(toUpdate.getId());
}
}
private Account load(Account toUpdate, Account.Id accountId, ReviewDb db) throws OrmException {
@@ -303,7 +300,6 @@ public class AccountManager {
}
byEmailCache.evict(account.getPreferredEmail());
byIdCache.evict(account.getId());
realm.onCreateAccount(who, account);
return new AuthResult(newId, extId.key(), true);
}
@@ -380,7 +376,6 @@ public class AccountManager {
if (a.getPreferredEmail() == null) {
a.setPreferredEmail(who.getEmailAddress());
accountsUpdateFactory.create().update(db, a);
byIdCache.evict(to);
}
byEmailCache.evict(who.getEmailAddress());
}
@@ -446,7 +441,6 @@ public class AccountManager {
&& a.getPreferredEmail().equals(who.getEmailAddress())) {
a.setPreferredEmail(null);
accountsUpdateFactory.create().update(db, a);
byIdCache.evict(from);
}
byEmailCache.evict(who.getEmailAddress());
}

42
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -50,26 +50,32 @@ public class AccountsUpdate {
*
* <p>The Gerrit server identity will be used as author and committer for all commits that update
* the accounts.
*
* <p>On updating accounts this class takes care to evict them from the account cache and thus
* triggers reindex for them.
*/
@Singleton
public static class Server {
private final GitRepositoryManager repoManager;
private final AccountCache accountCache;
private final AllUsersName allUsersName;
private final Provider<PersonIdent> serverIdent;
@Inject
public Server(
GitRepositoryManager repoManager,
AccountCache accountCache,
AllUsersName allUsersName,
@GerritPersonIdent Provider<PersonIdent> serverIdent) {
this.repoManager = repoManager;
this.accountCache = accountCache;
this.allUsersName = allUsersName;
this.serverIdent = serverIdent;
}
public AccountsUpdate create() {
PersonIdent i = serverIdent.get();
return new AccountsUpdate(repoManager, allUsersName, i, i);
return new AccountsUpdate(repoManager, accountCache, allUsersName, i, i);
}
}
@@ -82,6 +88,7 @@ public class AccountsUpdate {
@Singleton
public static class User {
private final GitRepositoryManager repoManager;
private final AccountCache accountCache;
private final AllUsersName allUsersName;
private final Provider<PersonIdent> serverIdent;
private final Provider<IdentifiedUser> identifiedUser;
@@ -89,10 +96,12 @@ public class AccountsUpdate {
@Inject
public User(
GitRepositoryManager repoManager,
AccountCache accountCache,
AllUsersName allUsersName,
@GerritPersonIdent Provider<PersonIdent> serverIdent,
Provider<IdentifiedUser> identifiedUser) {
this.repoManager = repoManager;
this.accountCache = accountCache;
this.allUsersName = allUsersName;
this.serverIdent = serverIdent;
this.identifiedUser = identifiedUser;
@@ -101,7 +110,7 @@ public class AccountsUpdate {
public AccountsUpdate create() {
PersonIdent i = serverIdent.get();
return new AccountsUpdate(
repoManager, allUsersName, createPersonIdent(i, identifiedUser.get()), i);
repoManager, accountCache, allUsersName, createPersonIdent(i, identifiedUser.get()), i);
}
private PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) {
@@ -110,16 +119,19 @@ public class AccountsUpdate {
}
private final GitRepositoryManager repoManager;
private final AccountCache accountCache;
private final AllUsersName allUsersName;
private final PersonIdent committerIdent;
private final PersonIdent authorIdent;
private AccountsUpdate(
GitRepositoryManager repoManager,
AccountCache accountCache,
AllUsersName allUsersName,
PersonIdent committerIdent,
PersonIdent authorIdent) {
this.repoManager = checkNotNull(repoManager, "repoManager");
this.accountCache = checkNotNull(accountCache, "accountCache");
this.allUsersName = checkNotNull(allUsersName, "allUsersName");
this.committerIdent = checkNotNull(committerIdent, "committerIdent");
this.authorIdent = checkNotNull(authorIdent, "authorIdent");
@@ -134,6 +146,7 @@ public class AccountsUpdate {
public void insert(ReviewDb db, Account account) throws OrmException, IOException {
db.accounts().insert(ImmutableSet.of(account));
createUserBranch(account);
accountCache.evict(account.getId());
}
/**
@@ -144,11 +157,13 @@ public class AccountsUpdate {
public void upsert(ReviewDb db, Account account) throws OrmException, IOException {
db.accounts().upsert(ImmutableSet.of(account));
createUserBranchIfNeeded(account);
accountCache.evict(account.getId());
}
/** Updates the account. */
public void update(ReviewDb db, Account account) throws OrmException {
public void update(ReviewDb db, Account account) throws OrmException, IOException {
db.accounts().update(ImmutableSet.of(account));
accountCache.evict(account.getId());
}
/**
@@ -161,26 +176,31 @@ public class AccountsUpdate {
* @throws OrmException if updating the account fails
*/
public Account atomicUpdate(ReviewDb db, Account.Id accountId, Consumer<Account> consumer)
throws OrmException {
return db.accounts()
.atomicUpdate(
accountId,
a -> {
consumer.accept(a);
return a;
});
throws OrmException, IOException {
Account account =
db.accounts()
.atomicUpdate(
accountId,
a -> {
consumer.accept(a);
return a;
});
accountCache.evict(accountId);
return account;
}
/** Deletes the account. */
public void delete(ReviewDb db, Account account) throws OrmException, IOException {
db.accounts().delete(ImmutableSet.of(account));
deleteUserBranch(account.getId());
accountCache.evict(account.getId());
}
/** Deletes the account. */
public void deleteByKey(ReviewDb db, Account.Id accountId) throws OrmException, IOException {
db.accounts().deleteKeys(ImmutableSet.of(accountId));
deleteUserBranch(accountId);
accountCache.evict(accountId);
}
private void createUserBranch(Account account) throws IOException {

5
gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java
View File

@@ -41,7 +41,6 @@ import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.account.externalids.ExternalIdsUpdate;
import com.google.gerrit.server.api.accounts.AccountExternalIdCreator;
import com.google.gerrit.server.group.GroupsCollection;
import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
import com.google.gerrit.server.ssh.SshKeyCache;
import com.google.gwtorm.server.OrmDuplicateKeyException;
@@ -70,7 +69,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
private final SshKeyCache sshKeyCache;
private final AccountCache accountCache;
private final AccountsUpdate.User accountsUpdate;
private final AccountIndexer indexer;
private final AccountByEmailCache byEmailCache;
private final AccountLoader.Factory infoLoader;
private final DynamicSet<AccountExternalIdCreator> externalIdCreators;
@@ -89,7 +87,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
SshKeyCache sshKeyCache,
AccountCache accountCache,
AccountsUpdate.User accountsUpdate,
AccountIndexer indexer,
AccountByEmailCache byEmailCache,
AccountLoader.Factory infoLoader,
DynamicSet<AccountExternalIdCreator> externalIdCreators,
@@ -105,7 +102,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
this.sshKeyCache = sshKeyCache;
this.accountCache = accountCache;
this.accountsUpdate = accountsUpdate;
this.indexer = indexer;
this.byEmailCache = byEmailCache;
this.infoLoader = infoLoader;
this.externalIdCreators = externalIdCreators;
@@ -198,7 +194,6 @@ public class CreateAccount implements RestModifyView<TopLevelResource, AccountIn
accountCache.evictByUsername(username);
byEmailCache.evict(input.email);
indexer.index(id);
AccountLoader loader = infoLoader.create(true);
AccountInfo info = loader.get(id);

4
gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java
View File

@@ -39,18 +39,15 @@ public class DeleteActive implements RestModifyView<AccountResource, Input> {
private final Provider<ReviewDb> dbProvider;
private final AccountsUpdate.Server accountsUpdate;
private final AccountCache byIdCache;
private final Provider<IdentifiedUser> self;
@Inject
DeleteActive(
Provider<ReviewDb> dbProvider,
AccountsUpdate.Server accountsUpdate,
AccountCache byIdCache,
Provider<IdentifiedUser> self) {
this.dbProvider = dbProvider;
this.accountsUpdate = accountsUpdate;
this.byIdCache = byIdCache;
this.self = self;
}
@@ -81,7 +78,6 @@ public class DeleteActive implements RestModifyView<AccountResource, Input> {
if (alreadyInactive.get()) {
throw new ResourceConflictException("account not active");
}
byIdCache.evict(account.getId());
return Response.none();
}
}

6
gerrit-server/src/main/java/com/google/gerrit/server/account/PutActive.java
View File

@@ -36,14 +36,11 @@ public class PutActive implements RestModifyView<AccountResource, Input> {
private final Provider<ReviewDb> dbProvider;
private final AccountsUpdate.Server accountsUpdate;
private final AccountCache byIdCache;
@Inject
PutActive(
Provider<ReviewDb> dbProvider, AccountsUpdate.Server accountsUpdate, AccountCache byIdCache) {
PutActive(Provider<ReviewDb> dbProvider, AccountsUpdate.Server accountsUpdate) {
this.dbProvider = dbProvider;
this.accountsUpdate = accountsUpdate;
this.byIdCache = byIdCache;
}
@Override
@@ -66,7 +63,6 @@ public class PutActive implements RestModifyView<AccountResource, Input> {
if (account == null) {
throw new ResourceNotFoundException("account not found");
}
byIdCache.evict(account.getId());
return alreadyActive.get() ? Response.ok("") : Response.created("");
}
}

6
gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java
View File

@@ -47,7 +47,6 @@ public class PutName implements RestModifyView<AccountResource, Input> {
private final PermissionBackend permissionBackend;
private final Provider<ReviewDb> dbProvider;
private final AccountsUpdate.Server accountsUpdate;
private final AccountCache byIdCache;
@Inject
PutName(
@@ -55,14 +54,12 @@ public class PutName implements RestModifyView<AccountResource, Input> {
Realm realm,
PermissionBackend permissionBackend,
Provider<ReviewDb> dbProvider,
AccountsUpdate.Server accountsUpdate,
AccountCache byIdCache) {
AccountsUpdate.Server accountsUpdate) {
this.self = self;
this.realm = realm;
this.permissionBackend = permissionBackend;
this.dbProvider = dbProvider;
this.accountsUpdate = accountsUpdate;
this.byIdCache = byIdCache;
}
@Override
@@ -93,7 +90,6 @@ public class PutName implements RestModifyView<AccountResource, Input> {
if (account == null) {
throw new ResourceNotFoundException("account not found");
}
byIdCache.evict(account.getId());
return Strings.isNullOrEmpty(account.getFullName())
? Response.none()
: Response.ok(account.getFullName());

6
gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java
View File

@@ -41,20 +41,17 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
private final Provider<ReviewDb> dbProvider;
private final PermissionBackend permissionBackend;
private final AccountsUpdate.Server accountsUpdate;
private final AccountCache byIdCache;
@Inject
PutPreferred(
Provider<CurrentUser> self,
Provider<ReviewDb> dbProvider,
PermissionBackend permissionBackend,
AccountsUpdate.Server accountsUpdate,
AccountCache byIdCache) {
AccountsUpdate.Server accountsUpdate) {
this.self = self;
this.dbProvider = dbProvider;
this.permissionBackend = permissionBackend;
this.accountsUpdate = accountsUpdate;
this.byIdCache = byIdCache;
}
@Override
@@ -86,7 +83,6 @@ public class PutPreferred implements RestModifyView<AccountResource.Email, Input
if (account == null) {
throw new ResourceNotFoundException("account not found");
}
byIdCache.evict(account.getId());
return alreadyPreferred.get() ? Response.ok("") : Response.created("");
}
}

6
gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java
View File

@@ -50,20 +50,17 @@ public class PutStatus implements RestModifyView<AccountResource, Input> {
private final Provider<ReviewDb> dbProvider;
private final PermissionBackend permissionBackend;
private final AccountsUpdate.Server accountsUpdate;
private final AccountCache byIdCache;
@Inject
PutStatus(
Provider<CurrentUser> self,
Provider<ReviewDb> dbProvider,
PermissionBackend permissionBackend,
AccountsUpdate.Server accountsUpdate,
AccountCache byIdCache) {
AccountsUpdate.Server accountsUpdate) {
this.self = self;
this.dbProvider = dbProvider;
this.permissionBackend = permissionBackend;
this.accountsUpdate = accountsUpdate;
this.byIdCache = byIdCache;
}
@Override
@@ -93,7 +90,6 @@ public class PutStatus implements RestModifyView<AccountResource, Input> {
if (account == null) {
throw new ResourceNotFoundException("account not found");
}
byIdCache.evict(account.getId());
return Strings.isNullOrEmpty(account.getStatus())
? Response.none()
: Response.ok(account.getStatus());

5
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -82,7 +82,6 @@ import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.AccountsUpdate;
@@ -314,7 +313,6 @@ public class ReceiveCommits {
private final CommitValidators.Factory commitValidatorsFactory;
private final RefOperationValidators.Factory refValidatorsFactory;
private final TagCache tagCache;
private final AccountCache accountCache;
private final ChangeInserter.Factory changeInserterFactory;
private final RequestScopePropagator requestScopePropagator;
private final SshInfo sshInfo;
@@ -386,7 +384,6 @@ public class ReceiveCommits {
PatchSetUtil psUtil,
ProjectCache projectCache,
TagCache tagCache,
AccountCache accountCache,
@Nullable SearchingChangeCacheImpl changeCache,
ChangeInserter.Factory changeInserterFactory,
CommitValidators.Factory commitValidatorsFactory,
@@ -428,7 +425,6 @@ public class ReceiveCommits {
this.projectCache = projectCache;
this.canonicalWebUrl = canonicalWebUrl;
this.tagCache = tagCache;
this.accountCache = accountCache;
this.changeInserterFactory = changeInserterFactory;
this.commitValidatorsFactory = commitValidatorsFactory;
this.refValidatorsFactory = refValidatorsFactory;
@@ -2750,7 +2746,6 @@ public class ReceiveCommits {
a.setFullName(c.getCommitterIdent().getName());
accountsUpdate.create().update(db, a);
user.getAccount().setFullName(a.getFullName());
accountCache.evict(a.getId());
}
} catch (OrmException e) {
logWarn("Cannot default full_name", e);

4
gerrit-server/src/test/java/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java
View File

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
import static java.util.stream.Collectors.toList;
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.accounts.Accounts.QueryRequest;
@@ -386,7 +387,7 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
String newName = "Test User";
Account account = accounts.get(db, new Account.Id(user1._accountId));
account.setFullName(newName);
accountsUpdate.create().update(db, account);
db.accounts().update(ImmutableSet.of(account));
assertQuery("name:" + quote(user1.name), user1);
assertQuery("name:" + quote(newName));
@@ -473,7 +474,6 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
a.setPreferredEmail(email);
a.setActive(active);
accountsUpdate.create().update(db, a);
accountCache.evict(id);
return id;
}
}

1
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -2313,7 +2313,6 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
a.setPreferredEmail(email);
a.setActive(active);
accountsUpdate.create().update(db, a);
accountCache.evict(id);
return id;
}
}

1
gerrit-server/src/test/java/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java
View File

@@ -324,7 +324,6 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
a.setPreferredEmail(email);
a.setActive(active);
accountsUpdate.create().update(db, a);
accountCache.evict(id);
return id;
}
}