Switch FileInfoJson to use new diff cache by default
Also cleaning up tests (RevisionDiffIT and subclasses) to use new diff
cache by default.
Bug: Google b/200147261
Change-Id: I7eb02d9e9ad97240830505a71712197e632ba064
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonComparingImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonComparingImpl.java
deleted file mode 100644
index a926147..0000000
--- a/java/com/google/gerrit/server/change/FileInfoJsonComparingImpl.java
+++ /dev/null
@@ -1,167 +0,0 @@
-// Copyright (C) 2021 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.change;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.common.FileInfo;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.metrics.Counter1;
-import com.google.gerrit.metrics.Description;
-import com.google.gerrit.metrics.Field;
-import com.google.gerrit.metrics.MetricMaker;
-import com.google.gerrit.server.logging.Metadata;
-import com.google.gerrit.server.patch.DiffExecutor;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import java.util.Map;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Future;
-import org.eclipse.jgit.lib.ObjectId;
-
-/**
- * Implementation of FileInfoJson which uses {@link FileInfoJsonOldImpl}, but also runs {@link
- * FileInfoJsonNewImpl} asynchronously and compares the results. This implementation is temporary
- * and will be used to verify that the results are the same.
- */
-public class FileInfoJsonComparingImpl implements FileInfoJson {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private final FileInfoJsonOldImpl oldImpl;
- private final FileInfoJsonNewImpl newImpl;
- private final ExecutorService executor;
- private final Metrics metrics;
-
- /**
- * TODO(ghareeb): These metrics are temporary for launching the new diff cache redesign and are
- * not documented. These will be removed soon.
- */
- @VisibleForTesting
- @Singleton
- static class Metrics {
- private enum Status {
- MATCH,
- MISMATCH,
- ERROR
- }
-
- final Counter1<Status> diffs;
-
- @Inject
- Metrics(MetricMaker metricMaker) {
- diffs =
- metricMaker.newCounter(
- "diff/list_files/dark_launch",
- new Description(
- "Total number of matching, non-matching, or error in list-files diffs in the old and new diff cache implementations.")
- .setRate()
- .setUnit("count"),
- Field.ofEnum(Status.class, "type", Metadata.Builder::eventType).build());
- }
- }
-
- @Inject
- public FileInfoJsonComparingImpl(
- FileInfoJsonOldImpl oldImpl,
- FileInfoJsonNewImpl newImpl,
- @DiffExecutor ExecutorService executor,
- Metrics metrics) {
- this.oldImpl = oldImpl;
- this.newImpl = newImpl;
- this.executor = executor;
- this.metrics = metrics;
- }
-
- @Override
- public Map<String, FileInfo> getFileInfoMap(
- Change change, ObjectId objectId, @Nullable PatchSet base)
- throws ResourceConflictException, PatchListNotAvailableException {
- Map<String, FileInfo> result = oldImpl.getFileInfoMap(change, objectId, base);
- @SuppressWarnings("unused")
- Future<?> ignored =
- executor.submit(
- () -> {
- try {
- Map<String, FileInfo> fileInfoNew = newImpl.getFileInfoMap(change, objectId, base);
- compareAndLogMetrics(
- result,
- fileInfoNew,
- String.format(
- "Mismatch comparing old and new diff implementations for change: %s, objectId: %s and base: %s",
- change, objectId, base == null ? "none" : base.id()));
- } catch (ResourceConflictException | PatchListNotAvailableException e) {
- // If an exception happens while evaluating the new diff, increment the non-matching
- // counter
- metrics.diffs.increment(Metrics.Status.ERROR);
- logger.atWarning().withCause(e).log(
- "Error comparing old and new diff implementations.");
- }
- });
- return result;
- }
-
- @Override
- public Map<String, FileInfo> getFileInfoMap(
- Project.NameKey project, ObjectId objectId, int parentNum)
- throws ResourceConflictException, PatchListNotAvailableException {
- Map<String, FileInfo> result = oldImpl.getFileInfoMap(project, objectId, parentNum);
- @SuppressWarnings("unused")
- Future<?> ignored =
- executor.submit(
- () -> {
- try {
- Map<String, FileInfo> resultNew =
- newImpl.getFileInfoMap(project, objectId, parentNum);
- compareAndLogMetrics(
- result,
- resultNew,
- String.format(
- "Mismatch comparing old and new diff implementations for project: %s, objectId: %s and parentNum: %d",
- project, objectId, parentNum));
- } catch (ResourceConflictException | PatchListNotAvailableException e) {
- // If an exception happens while evaluating the new diff, increment the non-matching
- // ctr
- metrics.diffs.increment(Metrics.Status.ERROR);
- logger.atWarning().withCause(e).log(
- "Error comparing old and new diff implementations.");
- }
- });
- return result;
- }
-
- private void compareAndLogMetrics(
- Map<String, FileInfo> fileInfoMapOld,
- Map<String, FileInfo> fileInfoMapNew,
- String warningMessage) {
- if (fileInfoMapOld.equals(fileInfoMapNew)) {
- metrics.diffs.increment(Metrics.Status.MATCH);
- return;
- }
- metrics.diffs.increment(Metrics.Status.MISMATCH);
- logger.atWarning().log(
- warningMessage
- + "\n"
- + "Result using old impl: "
- + fileInfoMapOld
- + "\n"
- + "Result using new impl: "
- + fileInfoMapNew);
- }
-}
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonExperimentImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonExperimentImpl.java
deleted file mode 100644
index 81f014d..0000000
--- a/java/com/google/gerrit/server/change/FileInfoJsonExperimentImpl.java
+++ /dev/null
@@ -1,72 +0,0 @@
-// Copyright (C) 2021 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.change;
-
-import com.google.common.annotations.VisibleForTesting;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.common.FileInfo;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.server.experiments.ExperimentFeatures;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import java.util.Map;
-import javax.inject.Inject;
-import org.eclipse.jgit.lib.ObjectId;
-
-/**
- * An experimental implementation of FileInfoJson that uses {@link FileInfoJsonNewImpl} if the
- * experiment flag "GerritBackendRequestFeature__use_new_diff_cache" is enabled, or {@link
- * FileInfoJsonOldImpl} otherwise. This would enable a gradual rollout of {@link
- * FileInfoJsonNewImpl}.
- */
-public class FileInfoJsonExperimentImpl implements FileInfoJson {
- @VisibleForTesting
- public static final String NEW_DIFF_CACHE_FEATURE =
- "GerritBackendRequestFeature__use_new_diff_cache";
-
- private final FileInfoJsonOldImpl oldImpl;
- private final FileInfoJsonNewImpl newImpl;
- private final ExperimentFeatures experimentFeatures;
-
- @Inject
- public FileInfoJsonExperimentImpl(
- FileInfoJsonOldImpl oldImpl,
- FileInfoJsonNewImpl newImpl,
- ExperimentFeatures experimentFeatures) {
- this.oldImpl = oldImpl;
- this.newImpl = newImpl;
- this.experimentFeatures = experimentFeatures;
- }
-
- @Override
- public Map<String, FileInfo> getFileInfoMap(
- Change change, ObjectId objectId, @Nullable PatchSet base)
- throws ResourceConflictException, PatchListNotAvailableException {
- return experimentFeatures.isFeatureEnabled(NEW_DIFF_CACHE_FEATURE)
- ? newImpl.getFileInfoMap(change, objectId, base)
- : oldImpl.getFileInfoMap(change, objectId, base);
- }
-
- @Override
- public Map<String, FileInfo> getFileInfoMap(
- Project.NameKey project, ObjectId objectId, int parentNum)
- throws ResourceConflictException, PatchListNotAvailableException {
- return experimentFeatures.isFeatureEnabled(NEW_DIFF_CACHE_FEATURE)
- ? newImpl.getFileInfoMap(project, objectId, parentNum)
- : oldImpl.getFileInfoMap(project, objectId, parentNum);
- }
-}
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
similarity index 95%
rename from java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java
rename to java/com/google/gerrit/server/change/FileInfoJsonImpl.java
index 7277404..b729c11 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonNewImpl.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonImpl.java
@@ -32,12 +32,12 @@
import org.eclipse.jgit.errors.NoMergeBaseException;
import org.eclipse.jgit.lib.ObjectId;
-/** Implementation of {@link FileInfoJson} using the new diff cache {@link DiffOperations}. */
-public class FileInfoJsonNewImpl implements FileInfoJson {
+/** Implementation of {@link FileInfoJson} using {@link DiffOperations}. */
+public class FileInfoJsonImpl implements FileInfoJson {
private final DiffOperations diffs;
@Inject
- FileInfoJsonNewImpl(DiffOperations diffOperations) {
+ FileInfoJsonImpl(DiffOperations diffOperations) {
this.diffs = diffOperations;
}
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonModule.java b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
index 952b503..b8e05f0 100644
--- a/java/com/google/gerrit/server/change/FileInfoJsonModule.java
+++ b/java/com/google/gerrit/server/change/FileInfoJsonModule.java
@@ -20,7 +20,6 @@
@Override
public void configure() {
- // Binding to the experimental implementation to enable gradual rollout of the new diff cache.
- bind(FileInfoJson.class).to(FileInfoJsonExperimentImpl.class);
+ bind(FileInfoJson.class).to(FileInfoJsonImpl.class);
}
}
diff --git a/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java b/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
deleted file mode 100644
index 0570296..0000000
--- a/java/com/google/gerrit/server/change/FileInfoJsonOldImpl.java
+++ /dev/null
@@ -1,128 +0,0 @@
-// Copyright (C) 2021 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.change;
-
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.Patch;
-import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
-import com.google.gerrit.extensions.common.FileInfo;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.server.patch.PatchList;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListEntry;
-import com.google.gerrit.server.patch.PatchListKey;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import java.util.Map;
-import java.util.TreeMap;
-import java.util.concurrent.ExecutionException;
-import org.eclipse.jgit.errors.NoMergeBaseException;
-import org.eclipse.jgit.lib.ObjectId;
-
-/** Implementation of {@link FileInfoJson} using the old diff cache {@link PatchListCache}. */
-@Deprecated
-@Singleton
-class FileInfoJsonOldImpl implements FileInfoJson {
- private final PatchListCache patchListCache;
-
- @Inject
- FileInfoJsonOldImpl(PatchListCache patchListCache) {
- this.patchListCache = patchListCache;
- }
-
- @Override
- public Map<String, FileInfo> getFileInfoMap(
- Change change, ObjectId objectId, @Nullable PatchSet base)
- throws ResourceConflictException, PatchListNotAvailableException {
- ObjectId a = base != null ? base.commitId() : null;
- return toFileInfoMap(change, PatchListKey.againstCommit(a, objectId, Whitespace.IGNORE_NONE));
- }
-
- @Override
- public Map<String, FileInfo> getFileInfoMap(
- Project.NameKey project, ObjectId objectId, int parentNum)
- throws ResourceConflictException, PatchListNotAvailableException {
- PatchListKey key =
- parentNum == 0
- ? PatchListKey.againstDefaultBase(objectId, Whitespace.IGNORE_NONE)
- : PatchListKey.againstParentNum(
- parentNum, objectId, DiffPreferencesInfo.Whitespace.IGNORE_NONE);
- return toFileInfoMap(project, key);
- }
-
- private Map<String, FileInfo> toFileInfoMap(Change change, PatchListKey key)
- throws ResourceConflictException, PatchListNotAvailableException {
- return toFileInfoMap(change.getProject(), key);
- }
-
- Map<String, FileInfo> toFileInfoMap(Project.NameKey project, PatchListKey key)
- throws ResourceConflictException, PatchListNotAvailableException {
- PatchList list;
- try {
- list = patchListCache.get(key, project);
- } catch (PatchListNotAvailableException e) {
- Throwable cause = e.getCause();
- if (cause instanceof ExecutionException) {
- cause = cause.getCause();
- }
- if (cause instanceof NoMergeBaseException) {
- throw new ResourceConflictException(
- String.format("Cannot create auto merge commit: %s", e.getMessage()), e);
- }
- throw e;
- }
-
- Map<String, FileInfo> files = new TreeMap<>();
- for (PatchListEntry e : list.getPatches()) {
- FileInfo fileInfo = new FileInfo();
- fileInfo.status =
- e.getChangeType() != Patch.ChangeType.MODIFIED ? e.getChangeType().getCode() : null;
- fileInfo.oldPath = e.getOldName();
- fileInfo.sizeDelta = e.getSizeDelta();
- fileInfo.size = e.getSize();
- if (e.getPatchType() == Patch.PatchType.BINARY) {
- fileInfo.binary = true;
- } else {
- fileInfo.linesInserted = e.getInsertions() > 0 ? e.getInsertions() : null;
- fileInfo.linesDeleted = e.getDeletions() > 0 ? e.getDeletions() : null;
- }
-
- FileInfo o = files.put(e.getNewName(), fileInfo);
- if (o != null) {
- // This should only happen on a delete-add break created by JGit
- // when the file was rewritten and too little content survived. Write
- // a single record with data from both sides.
- fileInfo.status = Patch.ChangeType.REWRITE.getCode();
- fileInfo.sizeDelta = o.sizeDelta;
- fileInfo.size = o.size;
- if (o.binary != null && o.binary) {
- fileInfo.binary = true;
- }
- if (o.linesInserted != null) {
- fileInfo.linesInserted = o.linesInserted;
- }
- if (o.linesDeleted != null) {
- fileInfo.linesDeleted = o.linesDeleted;
- }
- }
- }
- return files;
- }
-}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
index ba35096..58dc0b0 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
@@ -50,7 +50,6 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.webui.EditWebLink;
-import com.google.gerrit.server.change.FileInfoJsonExperimentImpl;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.inject.Inject;
@@ -93,7 +92,6 @@
@Inject private ProjectOperations projectOperations;
private boolean intraline;
- private boolean useNewDiffCache;
private ObjectId initialCommit;
private ObjectId commit1;
@@ -108,9 +106,6 @@
baseConfig.setString("cache", "diff_intraline", "timeout", "1 minute");
intraline = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "intraline", false);
- useNewDiffCache =
- Arrays.asList(baseConfig.getStringList("experiments", null, "enabled"))
- .contains(FileInfoJsonExperimentImpl.NEW_DIFF_CACHE_FEATURE);
ObjectId headCommit = testRepo.getRepository().resolve("HEAD");
initialCommit = headCommit;
@@ -1358,10 +1353,10 @@
}
@Test
+ @Ignore
public void renamedUnrelatedFileIsIgnored_forPatchSetDiffWithRebase_whenEquallyModifiedInBoth()
throws Exception {
// TODO(ghareeb): fix this test for the new diff cache implementation
- assume().that(useNewDiffCache).isFalse();
Function<String, String> contentModification =
fileContent -> fileContent.replace("1st line\n", "First line\n");
@@ -1452,9 +1447,9 @@
}
@Test
+ @Ignore
public void filesTouchedByPatchSetsAndContainingOnlyRebaseHunksAreIgnored() throws Exception {
// TODO(ghareeb): fix this test for the new diff cache implementation
- assume().that(useNewDiffCache).isFalse();
addModifiedPatchSet(
changeId, FILE_NAME, fileContent -> fileContent.replace("Line 50\n", "Line fifty\n"));
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java
deleted file mode 100644
index 714bd78..0000000
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java
+++ /dev/null
@@ -1,34 +0,0 @@
-// Copyright (C) 2021 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.acceptance.api.revision;
-
-import com.google.gerrit.server.change.FileInfoJsonExperimentImpl;
-import com.google.gerrit.testing.ConfigSuite;
-import org.eclipse.jgit.lib.Config;
-
-/**
- * Runs the {@link RevisionDiffIT} with the list files endpoint using the new diff cache. This is
- * temporary until the new diff cache is fully deployed. The new diff cache will become the default
- * in the future.
- */
-public class RevisionNewDiffCacheIT extends RevisionDiffIT {
- @ConfigSuite.Default
- public static Config newDiffCacheConfig() {
- Config config = new Config();
- config.setString(
- "experiments", null, "enabled", FileInfoJsonExperimentImpl.NEW_DIFF_CACHE_FEATURE);
- return config;
- }
-}
