AccountByEmailCacheImpl: Don't use account index to find accounts by email
Instead rather get accounts by email from either the database (if
external IDs are read from ReviewDb) or from the ExternalIdCache (if
external IDs are read from NoteDb).
For NoteDb Using the in-memory cache is faster than doing the lookup via
the account index.
To support querying accounts by email from ReviewDb the
account_external_ids_byEmail index is added back temporarily. The
follow-up change will drop the account_external_ids table completely and
hence this index will be gone again.
The ReviewDb method to query accounts by email is also needed to perform
the external ID migration on the https://*-review.googlesource.com
Gerrit servers at Google.
Change-Id: Ib9961060676c422b75c83b0cbc23c0e585813b16
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/AccountExternalIdAccess.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/AccountExternalIdAccess.java
index 9124301..e21faaf 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/AccountExternalIdAccess.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/AccountExternalIdAccess.java
@@ -30,6 +30,9 @@
@Query("WHERE accountId = ?")
ResultSet<AccountExternalId> byAccount(Account.Id id) throws OrmException;
+ @Query("WHERE emailAddress = ?")
+ ResultSet<AccountExternalId> byEmailAddress(String email) throws OrmException;
+
@Query
ResultSet<AccountExternalId> all() throws OrmException;
}
diff --git a/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_generic.sql b/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_generic.sql
index deceab9..2fcf53c 100644
--- a/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_generic.sql
+++ b/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_generic.sql
@@ -21,6 +21,9 @@
CREATE INDEX account_external_ids_byAccount
ON account_external_ids (account_id);
+-- covers: byEmailAddress
+CREATE INDEX account_external_ids_byEmail
+ON account_external_ids (email_address);
-- *********************************************************************
-- AccountGroupMemberAccess
diff --git a/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_maxdb.sql b/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_maxdb.sql
index 1ec8ea6..3be3c26 100644
--- a/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_maxdb.sql
+++ b/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_maxdb.sql
@@ -25,6 +25,10 @@
ON account_external_ids (account_id)
#
+-- covers: byEmailAddress
+CREATE INDEX account_external_ids_byEmail
+ON account_external_ids (email_address)
+#
-- *********************************************************************
-- AccountGroupMemberAccess
diff --git a/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_postgres.sql b/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_postgres.sql
index a11c86b..641c613 100644
--- a/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_postgres.sql
+++ b/gerrit-reviewdb/src/main/resources/com/google/gerrit/reviewdb/server/index_postgres.sql
@@ -68,6 +68,10 @@
CREATE INDEX account_external_ids_byAccount
ON account_external_ids (account_id);
+-- covers: byEmailAddress
+CREATE INDEX account_external_ids_byEmail
+ON account_external_ids (email_address);
+
-- *********************************************************************
-- AccountGroupMemberAccess
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java
index d45ecd8..950eac7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountByEmailCacheImpl.java
@@ -14,13 +14,15 @@
package com.google.gerrit.server.account;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Streams;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.query.account.InternalAccountQuery;
import com.google.gwtorm.server.SchemaFactory;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -29,7 +31,6 @@
import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
import java.util.Collections;
-import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import org.slf4j.Logger;
@@ -78,32 +79,23 @@
static class Loader extends CacheLoader<String, Set<Account.Id>> {
private final SchemaFactory<ReviewDb> schema;
- private final Provider<InternalAccountQuery> accountQueryProvider;
+
+ // This must be a provider to prevent a cyclic dependency within Google-internal glue code.
+ private final Provider<ExternalIds> externalIds;
@Inject
- Loader(SchemaFactory<ReviewDb> schema, Provider<InternalAccountQuery> accountQueryProvider) {
+ Loader(SchemaFactory<ReviewDb> schema, Provider<ExternalIds> externalIds) {
this.schema = schema;
- this.accountQueryProvider = accountQueryProvider;
+ this.externalIds = externalIds;
}
@Override
public Set<Account.Id> load(String email) throws Exception {
try (ReviewDb db = schema.open()) {
- Set<Account.Id> r = new HashSet<>();
- for (Account a : db.accounts().byPreferredEmail(email)) {
- r.add(a.getId());
- }
- for (AccountState accountState : accountQueryProvider.get().byEmailPrefix(email)) {
- if (accountState
- .getExternalIds()
- .stream()
- .filter(e -> email.equals(e.email()))
- .findAny()
- .isPresent()) {
- r.add(accountState.getAccount().getId());
- }
- }
- return ImmutableSet.copyOf(r);
+ return Streams.concat(
+ Streams.stream(db.accounts().byPreferredEmail(email)).map(a -> a.getId()),
+ externalIds.get().byEmail(db, email).stream().map(e -> e.accountId()))
+ .collect(toImmutableSet());
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java
index 484c246..8e7a12e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/DisabledExternalIdCache.java
@@ -17,6 +17,7 @@
import com.google.gerrit.reviewdb.client.Account;
import com.google.inject.AbstractModule;
import com.google.inject.Module;
+import java.io.IOException;
import java.util.Set;
import org.eclipse.jgit.lib.ObjectId;
@@ -73,4 +74,9 @@
public Set<ExternalId> byAccount(Account.Id accountId) {
throw new UnsupportedOperationException();
}
+
+ @Override
+ public Set<ExternalId> byEmail(String email) throws IOException {
+ throw new UnsupportedOperationException();
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
index d8171dd..ac2c279 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
@@ -57,6 +57,8 @@
Set<ExternalId> byAccount(Account.Id accountId) throws IOException;
+ Set<ExternalId> byEmail(String email) throws IOException;
+
default void onCreate(ObjectId newNotesRev, ExternalId extId) throws IOException {
onCreate(newNotesRev, Collections.singleton(extId));
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java
index 7eb29da..7fb61fc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIdCacheImpl.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.account.externalids;
+import static java.util.stream.Collectors.toSet;
+
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.collect.ImmutableSetMultimap;
@@ -220,7 +222,22 @@
try {
return extIdsByAccount.get(externalIdReader.readRevision()).get(accountId);
} catch (ExecutionException e) {
- log.warn("Cannot list external ids", e);
+ log.warn("Cannot list external ids by account", e);
+ return Collections.emptySet();
+ }
+ }
+
+ @Override
+ public Set<ExternalId> byEmail(String email) throws IOException {
+ try {
+ return extIdsByAccount
+ .get(externalIdReader.readRevision())
+ .values()
+ .stream()
+ .filter(e -> email.equals(e.email()))
+ .collect(toSet());
+ } catch (ExecutionException e) {
+ log.warn("Cannot list external ids by email", e);
return Collections.emptySet();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIds.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIds.java
index e8fb586..b77fed8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIds.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalIds.java
@@ -82,4 +82,12 @@
throws IOException, OrmException {
return byAccount(db, accountId).stream().filter(e -> e.key().isScheme(scheme)).collect(toSet());
}
+
+ public Set<ExternalId> byEmail(ReviewDb db, String email) throws IOException, OrmException {
+ if (externalIdReader.readFromGit()) {
+ return externalIdCache.byEmail(email);
+ }
+
+ return ExternalId.from(db.accountExternalIds().byEmailAddress(email).toList());
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
index d9e5d2f..2c42502 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/InternalAccountQuery.java
@@ -68,10 +68,6 @@
return query(AccountPredicates.defaultPredicate(query));
}
- public List<AccountState> byEmailPrefix(String emailPrefix) throws OrmException {
- return query(AccountPredicates.email(emailPrefix));
- }
-
public List<AccountState> byExternalId(String scheme, String id) throws OrmException {
return byExternalId(ExternalId.Key.create(scheme, id));
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
index f5a4dd6..caeba3a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
@@ -35,7 +35,7 @@
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
- public static final Class<Schema_144> C = Schema_144.class;
+ public static final Class<Schema_145> C = Schema_145.class;
public static int getBinaryVersion() {
return guessVersion(C);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_145.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_145.java
new file mode 100644
index 0000000..20acd32
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_145.java
@@ -0,0 +1,46 @@
+// 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.schema;
+
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gwtorm.jdbc.JdbcSchema;
+import com.google.gwtorm.schema.sql.SqlDialect;
+import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.StatementExecutor;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.sql.SQLException;
+
+/** Create account_external_ids_byEmail index. */
+public class Schema_145 extends SchemaVersion {
+
+ @Inject
+ Schema_145(Provider<Schema_144> prior) {
+ super(prior);
+ }
+
+ @Override
+ protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException, SQLException {
+ JdbcSchema schema = (JdbcSchema) db;
+ SqlDialect dialect = schema.getDialect();
+ try (StatementExecutor e = newExecutor(db)) {
+ dialect.dropIndex(e, "account_external_ids", "account_external_ids_byEmail");
+ e.execute(
+ "CREATE INDEX account_external_ids_byEmail"
+ + " ON account_external_ids"
+ + " (email_address)");
+ }
+ }
+}
