Fix ExternalId differential loading when cache is inconsistent
JGit may return a failure to update a repository, even if it actually
succeeded.
This can create an issue when loading externalIds, since
we would retry to insert the same value into the cache twice,
due to the retry performed by the client, leading the cache
loader to consistently blow up.
For example:
Ext-Id1:
* attempt to insert into All-Users: failed on JGit but not on disk [1]
* the key Ext-Id1 is stored in cache
Ext-Id1:
* retry to insert into All-Users, because of JGit failure
* try to add key Ext-Id1 entry BUT it already exists in cache => BOOM
Make differential loading idempotent to avoid failures due to retry to
insert the same entry again.
Make "buildAllExternalIds" package-private for testing visibility.
[1]: https://bugs.eclipse.org/bugs/show_bug.cgi?id=582044
Bug: Issue 16384
Release-Notes: Fix ExternalId differential loading when cache is inconsistent
Change-Id: Iba6d538451994045b4455829d995dadd38866332
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java
index 8b53d70..bf281a5 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCacheLoader.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.account.externalids;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CharMatcher;
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableMap;
@@ -213,18 +214,25 @@
*
* <p>Removals are applied before additions.
*
+ * <p>This method is accessible in tests to simulate an inconsistent cache status. It wouldn't be
+ * possible to simulate it by invoking "buildAllExternalIds" from the caller
+ * ExternalIdCacheLoader#load.
+ *
* @param repo open repository
* @param oldExternalIds prior state that is used as base
* @param additions map of name to blob ID for each external ID that should be added
* @param removals set of name {@link ObjectId}s that should be removed
*/
- private AllExternalIds buildAllExternalIds(
+ @VisibleForTesting
+ AllExternalIds buildAllExternalIds(
Repository repo,
AllExternalIds oldExternalIds,
Map<ObjectId, ObjectId> additions,
Set<ObjectId> removals)
throws IOException {
- ImmutableMap.Builder<ExternalId.Key, ExternalId> byKey = ImmutableMap.builder();
+ // Make sure the addition of external-ids deltas is idempotent in the
+ // key -> external-id map, allowing retry cycles
+ HashMap<ExternalId.Key, ExternalId> byKeyMutableMap = new HashMap<>();
ImmutableSetMultimap.Builder<Account.Id, ExternalId> byAccount = ImmutableSetMultimap.builder();
ImmutableSetMultimap.Builder<String, ExternalId> byEmail = ImmutableSetMultimap.builder();
@@ -234,7 +242,7 @@
continue;
}
- byKey.put(externalId.key(), externalId);
+ byKeyMutableMap.put(externalId.key(), externalId);
byAccount.put(externalId.accountId(), externalId);
if (externalId.email() != null) {
byEmail.put(externalId.email(), externalId);
@@ -257,14 +265,17 @@
continue;
}
- byKey.put(parsedExternalId.key(), parsedExternalId);
+ byKeyMutableMap.put(parsedExternalId.key(), parsedExternalId);
byAccount.put(parsedExternalId.accountId(), parsedExternalId);
if (parsedExternalId.email() != null) {
byEmail.put(parsedExternalId.email(), parsedExternalId);
}
}
}
- return AllExternalIds.create(byKey.build(), byAccount.build(), byEmail.build());
+ return AllExternalIds.create(
+ ImmutableMap.<ExternalId.Key, ExternalId>builder().putAll(byKeyMutableMap).build(),
+ byAccount.build(),
+ byEmail.build());
}
private AllExternalIds reloadAllExternalIds(ObjectId notesRev)
diff --git a/javatests/com/google/gerrit/server/account/externalids/ExternalIDCacheLoaderTest.java b/javatests/com/google/gerrit/server/account/externalids/ExternalIDCacheLoaderTest.java
index 0a874db..d270138 100644
--- a/javatests/com/google/gerrit/server/account/externalids/ExternalIDCacheLoaderTest.java
+++ b/javatests/com/google/gerrit/server/account/externalids/ExternalIDCacheLoaderTest.java
@@ -19,6 +19,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
+import com.google.common.base.CharMatcher;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import com.google.common.collect.ImmutableList;
@@ -34,7 +35,10 @@
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.testing.InMemoryRepositoryManager;
import java.io.IOException;
+import java.util.HashMap;
+import java.util.HashSet;
import java.util.function.Consumer;
+import java.util.stream.Stream;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
@@ -92,6 +96,36 @@
}
@Test
+ public void loadCacheSuccessfullyWhenInInconsistentState() throws Exception {
+ int key = 1;
+ int account = 1;
+ ExternalId externalId = externalId(key, account);
+ ExternalId.Key externalIdKey = externalId.key();
+
+ Repository repo = repoManager.openRepository(ALL_USERS);
+ ObjectId newState = insertExternalId(key, account);
+ TreeWalk tw = new TreeWalk(repo);
+ tw.reset(new RevWalk(repo).parseCommit(newState).getTree());
+ tw.next();
+
+ HashMap<ObjectId, ObjectId> additions = new HashMap<>();
+ additions.put(fileNameToObjectId(tw.getPathString()), tw.getObjectId(0));
+ AllExternalIds oldExternalIds =
+ AllExternalIds.create(Stream.<ExternalId>builder().add(externalId).build());
+
+ AllExternalIds allExternalIds =
+ loader.buildAllExternalIds(repo, oldExternalIds, additions, new HashSet<>());
+
+ assertThat(allExternalIds).isNotNull();
+ assertThat(allExternalIds.byKey().containsKey(externalIdKey)).isTrue();
+ assertThat(allExternalIds.byKey().get(externalIdKey)).isEqualTo(externalId);
+ }
+
+ private static ObjectId fileNameToObjectId(String path) {
+ return ObjectId.fromString(CharMatcher.is('/').removeFrom(path));
+ }
+
+ @Test
public void reloadsMultipleUpdatesUsingPartialReload() throws Exception {
ObjectId firstState = insertExternalId(1, 1);
insertExternalId(2, 2);
