opendev
/
gerrit
Code Issues Proposed changes

Optionally persist ExternalIdCache 

A significant source of latency on googlesource.com is reading in new
pack files from persistent storage to parse refs/meta/external-ids,
sometimes several seconds per load in the case of a virtual host with
thousands of users. This crops up when external IDs are updated as well
as on cold server starts. Both of these instances are pretty frequent:
external IDs are updated every time a new user registers, and servers
are typically updated daily. Serializing this cache is a relatively
cheap engineering cost to avoid these multi-second stalls, which are
quite annoying for users.

A server updating the external-ids ref bypasses a cache load, and
instead updates the state in-memory, then puts the result back into the
cache at the new ObjectId. When persisting the cache, this put also puts
the result into persistent storage. In a multi-master environment, there
is a race window between updating the ref value and putting the new
persistent object, which means other masters reading the ref value may
not find the persisted object. If multiple servers are receiving
concurrent requests, there's a good chance one or more masters will have
to re-load the data from NoteDb. This can be improved, but requires more
surgery to ExternalIdCache to replace the value in the cache prior to
committing the ref update.

Change-Id: I31d31d8ed490d01ce963a8162afba3daf9c1efff
This commit is contained in:
Dave Borowitz 2018-08-21 13:43:10 -07:00
parent c62a9a87ef
commit b20d23a8cd
5 changed files with 231 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
Documentation/config-gerrit.txt
View File

@ -786,6 +786,7 @@ Default is 128 MiB per cache, except:
+
* `"change_notes"`: disk storage is disabled by default
* `"diff_summary"`: default is `1g` (1 GiB of disk space)
* `"external_ids_map"`: disk storage is disabled by default
+
If 0 or negative, disk storage for the cache is disabled.
@ -862,8 +863,10 @@ of link:config-accounts.html#external-ids[all current external IDs]. The
cache may temporarily contain 2 entries, but the second one is promptly
expired.
+
It is not recommended to change the attributes of this cache away from
the defaults.
It is not recommended to change the in-memory attributes of this cache
away from the defaults. The cache may be persisted by setting
`diskLimit`, which is only recommended if cold start performance is
problematic.
cache `"git_tags"`::
+

61
java/com/google/gerrit/server/account/externalids/AllExternalIds.java
View File

@ -15,12 +15,18 @@
package com.google.gerrit.server.account.externalids;
import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
import static java.util.stream.Collectors.toList;
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.SetMultimap;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.cache.proto.Cache.AllExternalIdsProto;
import com.google.gerrit.server.cache.proto.Cache.AllExternalIdsProto.ExternalIdProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
import com.google.gerrit.server.cache.serialize.ProtoCacheSerializers;
import com.google.gerrit.server.cache.serialize.ProtoCacheSerializers.ObjectIdConverter;
import java.util.Collection;
/** Cache value containing all external IDs. */
@ -48,4 +54,59 @@ public abstract class AllExternalIds {
public abstract ImmutableSetMultimap<Account.Id, ExternalId> byAccount();
public abstract ImmutableSetMultimap<String, ExternalId> byEmail();
enum Serializer implements CacheSerializer<AllExternalIds> {
INSTANCE;
@Override
public byte[] serialize(AllExternalIds object) {
ObjectIdConverter idConverter = ObjectIdConverter.create();
AllExternalIdsProto.Builder allBuilder = AllExternalIdsProto.newBuilder();
object
.byAccount()
.values()
.stream()
.map(extId -> toProto(idConverter, extId))
.forEach(allBuilder::addExternalId);
return ProtoCacheSerializers.toByteArray(allBuilder.build());
}
private static ExternalIdProto toProto(ObjectIdConverter idConverter, ExternalId externalId) {
ExternalIdProto.Builder b =
ExternalIdProto.newBuilder()
.setKey(externalId.key().get())
.setAccountId(externalId.accountId().get());
if (externalId.email() != null) {
b.setEmail(externalId.email());
}
if (externalId.password() != null) {
b.setPassword(externalId.password());
}
if (externalId.blobId() != null) {
b.setBlobId(idConverter.toByteString(externalId.blobId()));
}
return b.build();
}
@Override
public AllExternalIds deserialize(byte[] in) {
ObjectIdConverter idConverter = ObjectIdConverter.create();
return create(
ProtoCacheSerializers.parseUnchecked(AllExternalIdsProto.parser(), in)
.getExternalIdList()
.stream()
.map(proto -> toExternalId(idConverter, proto))
.collect(toList()));
}
private static ExternalId toExternalId(ObjectIdConverter idConverter, ExternalIdProto proto) {
return ExternalId.create(
ExternalId.Key.parse(proto.getKey()),
new Account.Id(proto.getAccountId()),
// ExternalId treats null and empty strings the same, so no need to distinguish here.
proto.getEmail(),
proto.getPassword(),
!proto.getBlobId().isEmpty() ? idConverter.fromByteString(proto.getBlobId()) : null);
}
}
}

9
java/com/google/gerrit/server/account/externalids/ExternalIdModule.java
View File

@ -16,6 +16,7 @@ package com.google.gerrit.server.account.externalids;
import com.google.gerrit.server.account.externalids.ExternalIdCacheImpl.Loader;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.cache.serialize.ObjectIdCacheSerializer;
import com.google.inject.TypeLiteral;
import java.time.Duration;
import org.eclipse.jgit.lib.ObjectId;
@ -23,7 +24,7 @@ import org.eclipse.jgit.lib.ObjectId;
public class ExternalIdModule extends CacheModule {
@Override
protected void configure() {
cache(ExternalIdCacheImpl.CACHE_NAME, ObjectId.class, new TypeLiteral<AllExternalIds>() {})
persist(ExternalIdCacheImpl.CACHE_NAME, ObjectId.class, new TypeLiteral<AllExternalIds>() {})
// The cached data is potentially pretty large and we are always only interested
// in the latest value. However, due to a race condition, it is possible for different
// threads to observe different values of the meta ref, and hence request different keys
@ -32,7 +33,11 @@ public class ExternalIdModule extends CacheModule {
// memory.
.maximumWeight(2)
.expireFromMemoryAfterAccess(Duration.ofMinutes(1))
.loader(Loader.class);
.loader(Loader.class)
.diskLimit(-1)
.version(1)
.keySerializer(ObjectIdCacheSerializer.INSTANCE)
.valueSerializer(AllExternalIds.Serializer.INSTANCE);
bind(ExternalIdCacheImpl.class);
bind(ExternalIdCache.class).to(ExternalIdCacheImpl.class);

143
javatests/com/google/gerrit/server/account/externalids/AllExternalIdsTest.java Normal file
View File

@ -0,0 +1,143 @@
// Copyright (C) 2018 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.account.externalids;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.gerrit.server.cache.testing.CacheSerializerTestUtil.byteString;
import static com.google.gerrit.server.cache.testing.SerializedClassSubject.assertThatSerializedClass;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.externalids.AllExternalIds.Serializer;
import com.google.gerrit.server.cache.proto.Cache.AllExternalIdsProto;
import com.google.gerrit.server.cache.proto.Cache.AllExternalIdsProto.ExternalIdProto;
import com.google.inject.TypeLiteral;
import java.util.Arrays;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
public class AllExternalIdsTest {
@Test
public void serializeEmptyExternalIds() throws Exception {
assertRoundTrip(allExternalIds(), AllExternalIdsProto.getDefaultInstance());
}
@Test
public void serializeMultipleExternalIds() throws Exception {
Account.Id accountId1 = new Account.Id(1001);
Account.Id accountId2 = new Account.Id(1002);
assertRoundTrip(
allExternalIds(
ExternalId.create("scheme1", "id1", accountId1),
ExternalId.create("scheme2", "id2", accountId1),
ExternalId.create("scheme2", "id3", accountId2),
ExternalId.create("scheme3", "id4", accountId2)),
AllExternalIdsProto.newBuilder()
.addExternalId(
ExternalIdProto.newBuilder().setKey("scheme1:id1").setAccountId(1001).build())
.addExternalId(
ExternalIdProto.newBuilder().setKey("scheme2:id2").setAccountId(1001).build())
.addExternalId(
ExternalIdProto.newBuilder().setKey("scheme2:id3").setAccountId(1002).build())
.addExternalId(
ExternalIdProto.newBuilder().setKey("scheme3:id4").setAccountId(1002).build())
.build());
}
@Test
public void serializeExternalIdWithEmail() throws Exception {
assertRoundTrip(
allExternalIds(ExternalId.createEmail(new Account.Id(1001), "foo@example.com")),
AllExternalIdsProto.newBuilder()
.addExternalId(
ExternalIdProto.newBuilder()
.setKey("mailto:foo@example.com")
.setAccountId(1001)
.setEmail("foo@example.com"))
.build());
}
@Test
public void serializeExternalIdWithPassword() throws Exception {
assertRoundTrip(
allExternalIds(
ExternalId.create("scheme", "id", new Account.Id(1001), null, "hashed password")),
AllExternalIdsProto.newBuilder()
.addExternalId(
ExternalIdProto.newBuilder()
.setKey("scheme:id")
.setAccountId(1001)
.setPassword("hashed password"))
.build());
}
@Test
public void serializeExternalIdWithBlobId() throws Exception {
assertRoundTrip(
allExternalIds(
ExternalId.create(
ExternalId.create("scheme", "id", new Account.Id(1001)),
ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))),
AllExternalIdsProto.newBuilder()
.addExternalId(
ExternalIdProto.newBuilder()
.setKey("scheme:id")
.setAccountId(1001)
.setBlobId(
byteString(
0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef,
0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef)))
.build());
}
@Test
public void allExternalIdsMethods() {
assertThatSerializedClass(AllExternalIds.class)
.hasAutoValueMethods(
ImmutableMap.of(
"byAccount",
new TypeLiteral<ImmutableSetMultimap<Account.Id, ExternalId>>() {}.getType(),
"byEmail",
new TypeLiteral<ImmutableSetMultimap<String, ExternalId>>() {}.getType()));
}
@Test
public void externalIdMethods() {
assertThatSerializedClass(ExternalId.class)
.hasAutoValueMethods(
ImmutableMap.of(
"key", ExternalId.Key.class,
"accountId", Account.Id.class,
"email", String.class,
"password", String.class,
"blobId", ObjectId.class));
}
private static AllExternalIds allExternalIds(ExternalId... externalIds) {
return AllExternalIds.create(Arrays.asList(externalIds));
}
private static void assertRoundTrip(
AllExternalIds allExternalIds, AllExternalIdsProto expectedProto) throws Exception {
AllExternalIdsProto actualProto =
AllExternalIdsProto.parseFrom(Serializer.INSTANCE.serialize(allExternalIds));
assertThat(actualProto).ignoringRepeatedFieldOrder().isEqualTo(expectedProto);
AllExternalIds actual =
Serializer.INSTANCE.deserialize(Serializer.INSTANCE.serialize(allExternalIds));
assertThat(actual).isEqualTo(allExternalIds);
}
}

15
proto/cache.proto
View File

@ -219,3 +219,18 @@ message TagSetHolderProto {
}
TagSetProto tags = 2;
}
// Serialized form of
// com.google.gerrit.server.account.externalids.AllExternalIds.
// Next ID: 2
message AllExternalIdsProto {
// Next ID: 6
message ExternalIdProto {
string key = 1;
int32 accountId = 2;
string email = 3;
string password = 4;
bytes blobId = 5;
}
repeated ExternalIdProto external_id = 1;
}