opendev/gerrit
Code Issues Proposed changes

Let ExternalIdsUpdate take care to evict accounts from the account cache

Browse Source 
The account cache holds AccountState instances which contain the
external IDs of the accounts. Hence an account must be evicted from the
account cache when its external IDs are updated. At the moment it's the
responsibility of the caller to do the account eviction, but it can
easily be forgotten and it's more convenient if ExternalIdsUpdate takes
care of it. For some scenarios this may result in a few more cache
evictions (e.g. account creation), but for most operations the number of
account evictions should stay the same.

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

Change-Id: I1af02c7576eea81bb229a4663cb1e067ab137784
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2017-06-06 17:08:56 +02:00
parent fbcd038e53
commit 4847c3de4e
10 changed files with 39 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -29,7 +29,6 @@ import com.google.gerrit.server.account.ExternalId;
import com.google.gerrit.server.account.ExternalIdsUpdate;
import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
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;
@ -56,7 +55,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
@ -67,7 +65,6 @@ public class AccountCreator {
SshKeyCache sshKeyCache,
AccountCache accountCache,
AccountByEmailCache byEmailCache,
AccountIndexer indexer,
ExternalIdsUpdate.Server externalIdsUpdate) {
accounts = new HashMap<>();
reviewDbProvider = schema;
@ -76,7 +73,6 @@ public class AccountCreator {
this.sshKeyCache = sshKeyCache;
this.accountCache = accountCache;
this.byEmailCache = byEmailCache;
this.indexer = indexer;
this.externalIdsUpdate = externalIdsUpdate;
}
@ -120,11 +116,10 @@ public class AccountCreator {
sshKeyCache.evict(username);
}
accountCache.evict(id);
accountCache.evictByUsername(username);
byEmailCache.evict(email);
indexer.index(id);
account = new TestAccount(id, username, email, fullName, sshKey, httpPass);
accounts.put(username, account);
return account;

7
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@ -140,8 +140,6 @@ public class AccountIT extends AbstractDaemonTest {
externalIdsUpdate.delete(db, getExternalIds(user));
externalIdsUpdate.insert(db, savedExternalIds);
}
accountCache.evict(admin.getId());
accountCache.evict(user.getId());
}
@After
@ -455,7 +453,6 @@ public class AccountIT extends AbstractDaemonTest {
ExternalId.createWithEmail(ExternalId.Key.parse(extId1), admin.id, email),
ExternalId.createWithEmail(ExternalId.Key.parse(extId2), admin.id, email));
externalIdsUpdateFactory.create().insert(db, extIds);
accountCache.evict(admin.id);
assertThat(
gApi.accounts().self().getExternalIds().stream().map(e -> e.identity).collect(toSet()))
.containsAllOf(extId1, extId2);
@ -506,7 +503,6 @@ public class AccountIT extends AbstractDaemonTest {
externalIdsUpdateFactory
.create()
.insert(db, ExternalId.createWithEmail(ExternalId.Key.parse("foo:bar"), admin.id, email));
accountCache.evict(admin.id);
assertEmail(byEmailCache.get(email), admin);
// wrong case doesn't match
@ -714,7 +710,6 @@ public class AccountIT extends AbstractDaemonTest {
// Both users have a matching external ID for this key.
addExternalIdEmail(admin, "test5@example.com");
externalIdsUpdate.insert(db, ExternalId.create("foo", "myId", user.getId()));
accountCache.evict(user.getId());
TestKey key = validKeyWithSecondUserId();
addGpgKey(key.getPublicKeyArmored());
@ -940,8 +935,6 @@ public class AccountIT extends AbstractDaemonTest {
checkNotNull(email);
externalIdsUpdate.insert(
db, ExternalId.createWithEmail(name("test"), email, account.getId(), email));
// Clear saved AccountState and ExternalIds.
accountCache.evict(account.getId());
setApiUser(account);
}

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
View File

@ -191,6 +191,7 @@ public class ExternalIdIT extends AbstractDaemonTest {
ExternalIdsUpdate update =
new ExternalIdsUpdate(
repoManager,
accountCache,
allUsers,
serverIdent.get(),
serverIdent.get(),
@ -223,6 +224,7 @@ public class ExternalIdIT extends AbstractDaemonTest {
ExternalIdsUpdate update =
new ExternalIdsUpdate(
repoManager,
accountCache,
allUsers,
serverIdent.get(),
serverIdent.get(),

5
gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/DeleteGpgKey.java
View File

@ -25,7 +25,6 @@ import com.google.gerrit.gpg.PublicKeyStore;
import com.google.gerrit.gpg.server.DeleteGpgKey.Input;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.ExternalId;
import com.google.gerrit.server.account.ExternalIdsUpdate;
import com.google.gwtorm.server.OrmException;
@ -45,7 +44,6 @@ public class DeleteGpgKey implements RestModifyView<GpgKey, Input> {
private final Provider<PersonIdent> serverIdent;
private final Provider<ReviewDb> db;
private final Provider<PublicKeyStore> storeProvider;
private final AccountCache accountCache;
private final ExternalIdsUpdate.User externalIdsUpdateFactory;
@Inject
@ -53,12 +51,10 @@ public class DeleteGpgKey implements RestModifyView<GpgKey, Input> {
@GerritPersonIdent Provider<PersonIdent> serverIdent,
Provider<ReviewDb> db,
Provider<PublicKeyStore> storeProvider,
AccountCache accountCache,
ExternalIdsUpdate.User externalIdsUpdateFactory) {
this.serverIdent = serverIdent;
this.db = db;
this.storeProvider = storeProvider;
this.accountCache = accountCache;
this.externalIdsUpdateFactory = externalIdsUpdateFactory;
}
@ -74,7 +70,6 @@ public class DeleteGpgKey implements RestModifyView<GpgKey, Input> {
rsrc.getUser().getAccountId(),
ExternalId.Key.create(
SCHEME_GPGKEY, BaseEncoding.base16().encode(key.getFingerprint())));
accountCache.evict(rsrc.getUser().getAccountId());
try (PublicKeyStore store = storeProvider.get()) {
store.remove(rsrc.getKeyRing().getPublicKey().getFingerprint());

5
gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java
View File

@ -44,7 +44,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.ExternalId;
@ -89,7 +88,6 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
private final Provider<PublicKeyStore> storeProvider;
private final GerritPublicKeyChecker.Factory checkerFactory;
private final AddKeySender.Factory addKeyFactory;
private final AccountCache accountCache;
private final Provider<InternalAccountQuery> accountQueryProvider;
private final ExternalIdsUpdate.User externalIdsUpdateFactory;
@ -101,7 +99,6 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
Provider<PublicKeyStore> storeProvider,
GerritPublicKeyChecker.Factory checkerFactory,
AddKeySender.Factory addKeyFactory,
AccountCache accountCache,
Provider<InternalAccountQuery> accountQueryProvider,
ExternalIdsUpdate.User externalIdsUpdateFactory) {
this.serverIdent = serverIdent;
@ -110,7 +107,6 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
this.storeProvider = storeProvider;
this.checkerFactory = checkerFactory;
this.addKeyFactory = addKeyFactory;
this.accountCache = accountCache;
this.accountQueryProvider = accountQueryProvider;
this.externalIdsUpdateFactory = externalIdsUpdateFactory;
}
@ -148,7 +144,6 @@ public class PostGpgKeys implements RestModifyView<AccountResource, Input> {
externalIdsUpdateFactory
.create()
.replace(db.get(), rsrc.getUser().getAccountId(), extIdKeysToRemove, newExtIds);
accountCache.evict(rsrc.getUser().getAccountId());
return toJson(newKeys, toRemove, store, rsrc.getUser());
}
}

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

@ -407,7 +407,6 @@ public class GerritPublicKeyCheckerTest {
assertThat(store.save(cb)).isAnyOf(NEW, FAST_FORWARD, FORCED);
externalIdsUpdateFactory.create().insert(db, newExtIds);
accountCache.evict(user.getAccountId());
}
private TestKey add(TestKey k, IdentifiedUser user) throws Exception {

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

@ -374,13 +374,10 @@ public class AccountManager {
if (a.getPreferredEmail() == null) {
a.setPreferredEmail(who.getEmailAddress());
db.accounts().update(Collections.singleton(a));
byIdCache.evict(to);
}
}
if (who.getEmailAddress() != null) {
byEmailCache.evict(who.getEmailAddress());
}
byIdCache.evict(to);
}
return new AuthResult(to, who.getExternalIdKey(), false);
@ -449,9 +446,9 @@ public class AccountManager {
&& a.getPreferredEmail().equals(who.getEmailAddress())) {
a.setPreferredEmail(null);
db.accounts().update(Collections.singleton(a));
byIdCache.evict(from);
}
byEmailCache.evict(who.getEmailAddress());
byIdCache.evict(from);
}
} else {

1
gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java
View File

@ -120,7 +120,6 @@ public class ChangeUserName implements Callable<VoidResult> {
accountCache.evictByUsername(extId.key().id());
}
accountCache.evict(user.getAccountId());
accountCache.evictByUsername(newUsername);
sshKeyCache.evict(newUsername);
return VoidResult.INSTANCE;

37
gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdsUpdate.java
View File

@ -84,6 +84,9 @@ import org.eclipse.jgit.revwalk.RevWalk;
* </pre>
*
* For NoteDb each method call results in one commit on refs/meta/external-ids branch.
*
* <p>On updating external IDs this class takes care to evict affected accounts from the account
* cache and thus triggers reindex for them.
*/
public class ExternalIdsUpdate {
private static final String COMMIT_MSG = "Update external IDs";
@ -97,22 +100,25 @@ public class ExternalIdsUpdate {
@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 ExternalIdsUpdate create() {
PersonIdent i = serverIdent.get();
return new ExternalIdsUpdate(repoManager, allUsersName, i, i);
return new ExternalIdsUpdate(repoManager, accountCache, allUsersName, i, i);
}
}
@ -125,6 +131,7 @@ public class ExternalIdsUpdate {
@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;
@ -132,10 +139,12 @@ public class ExternalIdsUpdate {
@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;
@ -144,7 +153,7 @@ public class ExternalIdsUpdate {
public ExternalIdsUpdate create() {
PersonIdent i = serverIdent.get();
return new ExternalIdsUpdate(
repoManager, allUsersName, createPersonIdent(i, identifiedUser.get()), i);
repoManager, accountCache, allUsersName, createPersonIdent(i, identifiedUser.get()), i);
}
private PersonIdent createPersonIdent(PersonIdent ident, IdentifiedUser user) {
@ -166,6 +175,7 @@ public class ExternalIdsUpdate {
private static final Retryer<Void> RETRYER = retryerBuilder().build();
private final GitRepositoryManager repoManager;
private final AccountCache accountCache;
private final AllUsersName allUsersName;
private final PersonIdent committerIdent;
private final PersonIdent authorIdent;
@ -174,21 +184,31 @@ public class ExternalIdsUpdate {
private ExternalIdsUpdate(
GitRepositoryManager repoManager,
AccountCache accountCache,
AllUsersName allUsersName,
PersonIdent committerIdent,
PersonIdent authorIdent) {
this(repoManager, allUsersName, committerIdent, authorIdent, Runnables.doNothing(), RETRYER);
this(
repoManager,
accountCache,
allUsersName,
committerIdent,
authorIdent,
Runnables.doNothing(),
RETRYER);
}
@VisibleForTesting
public ExternalIdsUpdate(
GitRepositoryManager repoManager,
AccountCache accountCache,
AllUsersName allUsersName,
PersonIdent committerIdent,
PersonIdent authorIdent,
Runnable afterReadRevision,
Retryer<Void> retryer) {
this.repoManager = checkNotNull(repoManager, "repoManager");
this.accountCache = accountCache;
this.allUsersName = checkNotNull(allUsersName, "allUsersName");
this.committerIdent = checkNotNull(committerIdent, "committerIdent");
this.authorIdent = checkNotNull(authorIdent, "authorIdent");
@ -222,6 +242,7 @@ public class ExternalIdsUpdate {
insert(o.rw(), o.ins(), o.noteMap(), extId);
}
});
evictAccounts(extIds);
}
/**
@ -249,6 +270,7 @@ public class ExternalIdsUpdate {
upsert(o.rw(), o.ins(), o.noteMap(), extId);
}
});
evictAccounts(extIds);
}
/**
@ -279,6 +301,7 @@ public class ExternalIdsUpdate {
remove(o.rw(), o.noteMap(), extId);
}
});
evictAccounts(extIds);
}
/**
@ -308,6 +331,7 @@ public class ExternalIdsUpdate {
remove(o.rw(), o.noteMap(), accountId, extIdKey);
}
});
accountCache.evict(accountId);
}
/** Deletes all external IDs of the specified account. */
@ -348,6 +372,7 @@ public class ExternalIdsUpdate {
insert(o.rw(), o.ins(), o.noteMap(), extId);
}
});
accountCache.evict(accountId);
}
/**
@ -587,6 +612,12 @@ public class ExternalIdsUpdate {
return ins.insert(OBJ_TREE, new byte[] {});
}
private void evictAccounts(Collection<ExternalId> extIds) throws IOException {
for (Account.Id id : extIds.stream().map(ExternalId::accountId).collect(toSet())) {
accountCache.evict(id);
}
}
private static interface MyConsumer<T> {
void accept(T t) throws IOException, ConfigInvalidException, OrmException;
}

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

@ -54,18 +54,15 @@ public class PutHttpPassword implements RestModifyView<AccountResource, Input> {
private final Provider<CurrentUser> self;
private final Provider<ReviewDb> dbProvider;
private final AccountCache accountCache;
private final ExternalIdsUpdate.User externalIdsUpdate;
@Inject
PutHttpPassword(
Provider<CurrentUser> self,
Provider<ReviewDb> dbProvider,
AccountCache accountCache,
ExternalIdsUpdate.User externalIdsUpdate) {
this.self = self;
this.dbProvider = dbProvider;
this.accountCache = accountCache;
this.externalIdsUpdate = externalIdsUpdate;
}
@ -122,7 +119,6 @@ public class PutHttpPassword implements RestModifyView<AccountResource, Input> {
ExternalId newExtId =
ExternalId.createWithPassword(extId.key(), extId.accountId(), extId.email(), newPassword);
externalIdsUpdate.create().upsert(dbProvider.get(), newExtId);
accountCache.evict(user.getAccountId());
return Strings.isNullOrEmpty(newPassword) ? Response.<String>none() : Response.ok(newPassword);
}