Merge branch 'stable-3.10' into stable-3.11 * stable-3.10: Clarify constraints of project deletions Introduce projectVersion cache Rename getPathFromDynamoDB to getItemFromDynamoDB Handle project deletion Add visible-assertion as test dependency Change-Id: I1badfa76f731c1f6a39ce0d783783f4956e24b9a
diff --git a/BUILD b/BUILD index 1dbc913..f45d775 100644 --- a/BUILD +++ b/BUILD
@@ -53,13 +53,14 @@ ":aws-dynamodb-refdb__plugin", "//plugins/global-refdb", "@amazon-regions//jar", - "@testcontainers//jar", + "@aws-java-sdk-dynamodb//jar", "@docker-java-api//jar", "@docker-java-transport//jar", "@duct-tape//jar", "@jna//jar", "@testcontainer-localstack//jar", - "@aws-java-sdk-dynamodb//jar", + "@testcontainers//jar", + "@visible-assertions//jar", ], )
diff --git a/external_plugin_deps.bzl b/external_plugin_deps.bzl index c3aa3e8..8267357 100644 --- a/external_plugin_deps.bzl +++ b/external_plugin_deps.bzl
@@ -97,6 +97,12 @@ ) maven_jar( + name = "visible-assertions", + artifact = "org.rnorth.visible-assertions:visible-assertions:2.1.2", + sha1 = "20d31a578030ec8e941888537267d3123c2ad1c1", + ) + + maven_jar( name = "docker-java-api", artifact = "com.github.docker-java:docker-java-api:" + DOCKER_JAVA_VERS, sha1 = "4ac22a72d546a9f3523cd4b5fabffa77c4a6ec7c",
diff --git a/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabase.java b/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabase.java index 933fd3f..712f5d1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabase.java +++ b/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabase.java
@@ -14,6 +14,8 @@ package com.googlesource.gerrit.plugins.validation.dfsrefdb.dynamodb; +import static com.googlesource.gerrit.plugins.validation.dfsrefdb.dynamodb.ProjectVersionCacheModule.PROJECT_VERSION_CACHE; + import com.amazonaws.services.dynamodbv2.AcquireLockOptions; import com.amazonaws.services.dynamodbv2.AmazonDynamoDB; import com.amazonaws.services.dynamodbv2.AmazonDynamoDBLockClient; @@ -28,12 +30,18 @@ import com.gerritforge.gerrit.globalrefdb.ExtendedGlobalRefDatabase; import com.gerritforge.gerrit.globalrefdb.GlobalRefDbLockException; import com.gerritforge.gerrit.globalrefdb.GlobalRefDbSystemError; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableMap; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.Project.NameKey; import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.name.Named; import java.util.Optional; +import java.util.concurrent.ExecutionException; import javax.inject.Singleton; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; @@ -51,25 +59,36 @@ private final AmazonDynamoDBLockClient lockClient; private final AmazonDynamoDB dynamoDBClient; private final Configuration configuration; + private final LoadingCache<String, Optional<Integer>> projectVersionCache; @Inject DynamoDBRefDatabase( AmazonDynamoDBLockClient lockClient, AmazonDynamoDB dynamoDBClient, - Configuration configuration) { + Configuration configuration, + @Named(PROJECT_VERSION_CACHE) LoadingCache<String, Optional<Integer>> projectVersionCache) { this.lockClient = lockClient; this.dynamoDBClient = dynamoDBClient; this.configuration = configuration; + this.projectVersionCache = projectVersionCache; } - static String pathFor(Project.NameKey projectName, String refName) { - return "/" + projectName + "/" + refName; + String pathFor(Project.NameKey projectName, String refName) { + return versionPrefix(getCurrentVersion(projectName)) + "/" + projectName + "/" + refName; + } + + static String currentVersionKey(Project.NameKey projectName) { + return "|" + projectName; + } + + static String versionPrefix(Integer version) { + return version != null ? "|" + version : ""; } @Override public boolean isUpToDate(Project.NameKey project, Ref ref) throws GlobalRefDbLockException { try { - GetItemResult result = getPathFromDynamoDB(project, ref.getName()); + GetItemResult result = getItemFromDynamoDB(pathFor(project, ref.getName())); if (!exists(result)) { return true; } @@ -150,7 +169,10 @@ @Override public <T> void put(NameKey project, String refName, T value) throws GlobalRefDbSystemError { - String refPath = pathFor(project, refName); + doPut(project, pathFor(project, refName), value); + } + + public <T> void doPut(NameKey project, String refPath, T value) throws GlobalRefDbSystemError { String refValue = Optional.ofNullable(value).map(Object::toString).orElse(ObjectId.zeroId().getName()); try { @@ -197,7 +219,7 @@ @Override public boolean exists(Project.NameKey project, String refName) { try { - if (!exists(getPathFromDynamoDB(project, refName))) { + if (!exists(getItemFromDynamoDB(pathFor(project, refName)))) { logger.atFine().log("ref '%s' does not exist in dynamodb", pathFor(project, refName)); return false; } @@ -211,11 +233,24 @@ return false; } + @Nullable + public Integer getCurrentVersion(Project.NameKey project) throws GlobalRefDbSystemError { + try { + return projectVersionCache.get(project.get()).orElse(null); + } catch (ExecutionException e) { + throw new GlobalRefDbSystemError("Could not check project version", e); + } + } + @Override public void remove(Project.NameKey project) throws GlobalRefDbSystemError { - // TODO: to remove all refs related to project we'd need to be able to query - // dynamodb by 'project': perhaps we should have a composite key of: - // PK: project, SK: ref + Integer currentVersion = getCurrentVersion(project); + int nextVersion = (currentVersion != null ? currentVersion : 0) + 1; + + doPut(project, currentVersionKey(project), Integer.toString(nextVersion)); + logger.atWarning().log( + "Project %s removed, current version %s, next version %s", + project, currentVersion, nextVersion); } @SuppressWarnings("unchecked") @@ -223,7 +258,7 @@ public <T> Optional<T> get(Project.NameKey project, String refName, Class<T> clazz) throws GlobalRefDbSystemError { try { - GetItemResult item = getPathFromDynamoDB(project, refName); + GetItemResult item = getItemFromDynamoDB(pathFor(project, refName)); if (!exists(item)) { return Optional.empty(); } @@ -239,14 +274,41 @@ } } - private GetItemResult getPathFromDynamoDB(Project.NameKey project, String refName) { - return dynamoDBClient.getItem( - configuration.getRefsDbTableName(), - ImmutableMap.of(REF_DB_PRIMARY_KEY, new AttributeValue(pathFor(project, refName))), - true); + private GetItemResult getItemFromDynamoDB(String refPath) { + return getItemFromDynamoDB(refPath, true); } - private boolean exists(GetItemResult result) { + public GetItemResult getItemFromDynamoDB(String refPath, Boolean consistentRead) { + return dynamoDBClient.getItem( + configuration.getRefsDbTableName(), + ImmutableMap.of(REF_DB_PRIMARY_KEY, new AttributeValue(refPath)), + consistentRead); + } + + public boolean exists(GetItemResult result) { return result.getItem() != null && !result.getItem().isEmpty(); } + + static class ProjectVersionCacheLoader extends CacheLoader<String, Optional<Integer>> { + + private final Provider<DynamoDBRefDatabase> dynamoDBRefDatabaseProvider; + + @Inject + public ProjectVersionCacheLoader(Provider<DynamoDBRefDatabase> dynamoDBRefDatabaseProvider) { + this.dynamoDBRefDatabaseProvider = dynamoDBRefDatabaseProvider; + } + + @Override + public Optional<Integer> load(String project) throws Exception { + GetItemResult item = + dynamoDBRefDatabaseProvider + .get() + .getItemFromDynamoDB(currentVersionKey(Project.nameKey(project)), false); + Integer currentVersion = + dynamoDBRefDatabaseProvider.get().exists(item) + ? Integer.parseInt(item.getItem().get(REF_DB_VALUE_KEY).getS()) + : null; + return Optional.ofNullable(currentVersion); + } + } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/Module.java b/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/Module.java index 946d401..4def9f6 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/Module.java +++ b/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/Module.java
@@ -33,6 +33,7 @@ DynamicItem.bind(binder(), GlobalRefDatabase.class) .to(DynamoDBRefDatabase.class) .in(Scopes.SINGLETON); + install(new ProjectVersionCacheModule()); bind(AmazonDynamoDB.class).toProvider(AmazonDynamoDBProvider.class).in(SINGLETON); bind(AmazonDynamoDBLockClient.class).toProvider(DynamoDBLockClientProvider.class).in(SINGLETON); listener().to(DynamoDBLifeCycleManager.class);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/ProjectVersionCacheModule.java b/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/ProjectVersionCacheModule.java new file mode 100644 index 0000000..55c581c --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/ProjectVersionCacheModule.java
@@ -0,0 +1,32 @@ +// Copyright (C) 2024 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.googlesource.gerrit.plugins.validation.dfsrefdb.dynamodb; + +import com.google.gerrit.server.cache.CacheModule; +import com.google.inject.TypeLiteral; +import java.time.Duration; +import java.util.Optional; + +public class ProjectVersionCacheModule extends CacheModule { + + public static final String PROJECT_VERSION_CACHE = "projectVersion"; + + @Override + protected void configure() { + cache(PROJECT_VERSION_CACHE, String.class, new TypeLiteral<Optional<Integer>>() {}) + .expireAfterWrite(Duration.ofSeconds(60)) + .maximumWeight(Long.MAX_VALUE) + .loader(DynamoDBRefDatabase.ProjectVersionCacheLoader.class); + } +}
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 0d871cc..4f85f41 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -40,5 +40,19 @@ Default: When not specified credentials are provided via the Default Credentials Provider Chain, as explained [here](https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/credentials.html) +### Project Deletion +This plugin introduces a caching mechanism to reduce the number of requests to +DynamoDB by storing the current live version of a project. + +Checking the current live version of a project directly from DynamoDB can be +costly. To minimise the number of DynamoDb operations that are unlikely to +provide a different result, avoiding excessive charges for reading continuously +unchanged data, the plugin employs a cache with a time-to-live (TTL) of `60 +seconds`. + +> **IMPORTANT NOTE** When a project is deleted, it cannot be recreated with the +> same name for the subsequent 60 seconds, because of the of the _live version_ +> caching mechanism described above. Any attempt to recreate the same project +> with the same name *before 60 seconds* will result in a failure.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabaseIT.java b/src/test/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabaseIT.java index 5012658..246ff79 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabaseIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/validation/dfsrefdb/dynamodb/DynamoDBRefDatabaseIT.java
@@ -21,11 +21,15 @@ import com.amazonaws.services.dynamodbv2.AmazonDynamoDB; import com.gerritforge.gerrit.globalrefdb.GlobalRefDbLockException; +import com.google.common.cache.LoadingCache; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.TestPlugin; import com.google.gerrit.acceptance.WaitUtil; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Project; +import com.google.inject.Key; +import com.google.inject.TypeLiteral; +import com.google.inject.name.Names; import java.time.Duration; import java.util.Optional; import org.eclipse.jgit.lib.ObjectId; @@ -244,6 +248,34 @@ assertThat(dynamoDBRefDatabase().compareAndPut(project, refName, null, newRefValue)).isTrue(); } + @Test + public void projectVersionShouldBeUsedAsPrefix() { + String versionKey = DynamoDBRefDatabase.currentVersionKey(project); + dynamoDBRefDatabase().doPut(project, versionKey, "1"); + + assertThat(dynamoDBRefDatabase().pathFor(project, "refs/heads/master")) + .isEqualTo("|1/" + project + "/refs/heads/master"); + } + + @Test + public void removeProjectShouldIncreaseProjectVersionWhenNotCached() { + assertThat(dynamoDBRefDatabase().getCurrentVersion(project)).isNull(); + + dynamoDBRefDatabase().remove(project); + projectVersionCache().invalidate(project.get()); + + assertThat(dynamoDBRefDatabase().getCurrentVersion(project)).isEqualTo(1); + } + + @Test + public void removeProjectShouldKeepCurrentVersionWhenCached() { + assertThat(dynamoDBRefDatabase().getCurrentVersion(project)).isNull(); + + dynamoDBRefDatabase().remove(project); + + assertThat(dynamoDBRefDatabase().getCurrentVersion(project)).isNull(); + } + private AmazonDynamoDB dynamoDBClient() { return plugin.getSysInjector().getInstance(AmazonDynamoDB.class); } @@ -252,6 +284,15 @@ return plugin.getSysInjector().getInstance(DynamoDBRefDatabase.class); } + private LoadingCache<String, Optional<Integer>> projectVersionCache() { + return plugin + .getSysInjector() + .getInstance( + Key.get( + new TypeLiteral<>() {}, + Names.named(ProjectVersionCacheModule.PROJECT_VERSION_CACHE))); + } + private void createRefInDynamoDB(Project.NameKey project, String refPath, String refValue) { dynamoDBRefDatabase().put(project, refPath, refValue); }