From 02ac16d7af360b10cf4ddb92f47e194f900f0927 Mon Sep 17 00:00:00 2001 From: Youssef Elghareeb Date: Mon, 15 Mar 2021 17:39:53 +0100 Subject: [PATCH] Switch GetDiff to use the new diff cache Instead of running GetDiff using the old diff cache and sanity checking the results using the new diff cache, we switch the implementation to use the new diff cache, and temporariy keep running the old diff cache async to export metrics. The new logic in this change is still gated with a config (false as default) to rollout the new diff cache gradually on selective hosts. I also removed the use-new-diff-cache request parameter as this is not needed anymore (was used for debugging). Change-Id: I34e10b2165bf0669c736ab17ebc2a871ccfbb8bd --- .../server/change/FileInfoJsonModule.java | 4 +- .../server/patch/PatchScriptFactory.java | 93 ++++++++----------- .../patch/SubmitWithStickyApprovalDiff.java | 3 +- .../gerrit/server/restapi/change/GetDiff.java | 12 +-- .../api/revision/RevisionDiffIT.java | 13 +-- .../RevisionNewDiffCacheForSingleFileIT.java | 2 +- .../api/revision/RevisionNewDiffCacheIT.java | 7 +- 7 files changed, 60 insertions(+), 74 deletions(-) diff --git a/java/com/google/gerrit/server/change/FileInfoJsonModule.java b/java/com/google/gerrit/server/change/FileInfoJsonModule.java index 11d60d1f74..de116bbaa9 100644 --- a/java/com/google/gerrit/server/change/FileInfoJsonModule.java +++ b/java/com/google/gerrit/server/change/FileInfoJsonModule.java @@ -19,13 +19,15 @@ import com.google.inject.AbstractModule; import org.eclipse.jgit.lib.Config; public class FileInfoJsonModule extends AbstractModule { + /** Use the new diff cache implementation {@link FileInfoJsonNewImpl}. */ private final boolean useNewDiffCache; /** Used to dark launch the new diff cache with the list files endpoint. */ private final boolean runNewDiffCacheAsync; public FileInfoJsonModule(@GerritServerConfig Config cfg) { - this.useNewDiffCache = cfg.getBoolean("cache", "diff_cache", "useNewDiffCache", false); + this.useNewDiffCache = + cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false); this.runNewDiffCacheAsync = cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_listFiles", false); } diff --git a/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/java/com/google/gerrit/server/patch/PatchScriptFactory.java index 91dc6e3510..07b01c4563 100644 --- a/java/com/google/gerrit/server/patch/PatchScriptFactory.java +++ b/java/com/google/gerrit/server/patch/PatchScriptFactory.java @@ -82,8 +82,7 @@ public class PatchScriptFactory implements Callable { @Assisted("patchSetA") PatchSet.Id patchSetA, @Assisted("patchSetB") PatchSet.Id patchSetB, DiffPreferencesInfo diffPrefs, - CurrentUser currentUser, - boolean useNewDiffCache); + CurrentUser currentUser); PatchScriptFactory create( ChangeNotes notes, @@ -91,8 +90,7 @@ public class PatchScriptFactory implements Callable { int parentNum, PatchSet.Id patchSetB, DiffPreferencesInfo diffPrefs, - CurrentUser currentUser, - boolean useNewDiffCache); + CurrentUser currentUser); } /** These metrics are temporary for launching the new redesigned diff cache. */ @@ -139,10 +137,7 @@ public class PatchScriptFactory implements Callable { private ChangeNotes notes; - private final boolean runNewDiffCacheAsync; - - // TODO(ghareeb): temporary field used for testing. Please remove. - private final boolean useNewDiffCache; + private final boolean runNewDiffCache; @AssistedInject PatchScriptFactory( @@ -162,8 +157,7 @@ public class PatchScriptFactory implements Callable { @Assisted("patchSetA") @Nullable PatchSet.Id patchSetA, @Assisted("patchSetB") PatchSet.Id patchSetB, @Assisted DiffPreferencesInfo diffPrefs, - @Assisted CurrentUser currentUser, - @Assisted boolean useNewDiffCache) { + @Assisted CurrentUser currentUser) { this.repoManager = grm; this.psUtil = psUtil; this.builderFactory = builderFactory; @@ -183,9 +177,7 @@ public class PatchScriptFactory implements Callable { this.diffPrefs = diffPrefs; this.currentUser = currentUser; - this.runNewDiffCacheAsync = - cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_getDiff", false); - this.useNewDiffCache = useNewDiffCache; + this.runNewDiffCache = cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false); changeId = patchSetB.changeId(); } @@ -208,8 +200,7 @@ public class PatchScriptFactory implements Callable { @Assisted int parentNum, @Assisted PatchSet.Id patchSetB, @Assisted DiffPreferencesInfo diffPrefs, - @Assisted CurrentUser currentUser, - @Assisted boolean useNewDiffCache) { + @Assisted CurrentUser currentUser) { this.repoManager = grm; this.psUtil = psUtil; this.builderFactory = builderFactory; @@ -229,9 +220,7 @@ public class PatchScriptFactory implements Callable { this.diffPrefs = diffPrefs; this.currentUser = currentUser; - this.runNewDiffCacheAsync = - cfg.getBoolean("cache", "diff_cache", "runNewDiffCacheAsync_getDiff", false); - this.useNewDiffCache = useNewDiffCache; + this.runNewDiffCache = cfg.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false); changeId = patchSetB.changeId(); checkArgument(parentNum >= 0, "parentNum must be >= 0"); @@ -270,28 +259,15 @@ public class PatchScriptFactory implements Callable { } bId = edit.get().getEditCommit(); } - - if (useNewDiffCache) { - FileDiffOutput fileDiffOutput = - aId == null - ? diffOperations.getModifiedFileAgainstParent( - notes.getProjectName(), - bId, - parentNum == -1 ? null : parentNum + 1, - fileName, - diffPrefs.ignoreWhitespace) - : diffOperations.getModifiedFile( - notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace); - return newBuilder().toPatchScriptNew(git, fileDiffOutput); + if (runNewDiffCache) { + PatchScript patchScript = getPatchScriptWithNewDiffCache(git, aId, bId); + // TODO(ghareeb): remove the async run. This is temporarily used to keep sanity checking + // the results while rolling out the new diff cache. + runOldDiffCacheAsyncAndExportMetrics(git, aId, bId, patchScript); + return patchScript; + } else { + return getPatchScriptWithOldDiffCache(git, aId, bId); } - PatchScriptBuilder patchScriptBuilder = newBuilder(); - PatchList list = listFor(keyFor(aId, bId, diffPrefs.ignoreWhitespace)); - PatchListEntry content = list.get(fileName); - PatchScript patchScript = patchScriptBuilder.toPatchScriptOld(git, list, content); - if (runNewDiffCacheAsync) { - runNewDiffCacheAsyncAndExportMetrics(git, aId, bId, patchScript); - } - return patchScript; } catch (PatchListNotAvailableException e) { throw new NoSuchChangeException(changeId, e); } catch (DiffNotAvailableException e) { @@ -311,24 +287,14 @@ public class PatchScriptFactory implements Callable { } } - private void runNewDiffCacheAsyncAndExportMetrics( + private void runOldDiffCacheAsyncAndExportMetrics( Repository git, ObjectId aId, ObjectId bId, PatchScript expected) { @SuppressWarnings("unused") Future possiblyIgnoredError = executor.submit( () -> { try { - FileDiffOutput fileDiffOutput = - aId == null - ? diffOperations.getModifiedFileAgainstParent( - notes.getProjectName(), - bId, - parentNum == -1 ? null : parentNum + 1, - fileName, - diffPrefs.ignoreWhitespace) - : diffOperations.getModifiedFile( - notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace); - PatchScript patchScript = newBuilder().toPatchScriptNew(git, fileDiffOutput); + PatchScript patchScript = getPatchScriptWithOldDiffCache(git, aId, bId); if (areEqualPatchscripts(patchScript, expected)) { metrics.diffs.increment(metrics.MATCH); } else { @@ -337,7 +303,7 @@ public class PatchScriptFactory implements Callable { "Mismatching diff for change %s, old commit ID: %s, new commit ID: %s, file name: %s.", changeId.toString(), aId, bId, fileName); } - } catch (DiffNotAvailableException | IOException e) { + } catch (PatchListNotAvailableException | IOException e) { metrics.diffs.increment(metrics.ERROR); logger.atSevere().atMostEvery(10, TimeUnit.SECONDS).log( String.format( @@ -348,6 +314,29 @@ public class PatchScriptFactory implements Callable { }); } + private PatchScript getPatchScriptWithOldDiffCache(Repository git, ObjectId aId, ObjectId bId) + throws IOException, PatchListNotAvailableException { + PatchScriptBuilder patchScriptBuilder = newBuilder(); + PatchList list = listFor(keyFor(aId, bId, diffPrefs.ignoreWhitespace)); + PatchListEntry content = list.get(fileName); + return patchScriptBuilder.toPatchScriptOld(git, list, content); + } + + private PatchScript getPatchScriptWithNewDiffCache(Repository git, ObjectId aId, ObjectId bId) + throws IOException, DiffNotAvailableException { + FileDiffOutput fileDiffOutput = + aId == null + ? diffOperations.getModifiedFileAgainstParent( + notes.getProjectName(), + bId, + parentNum == -1 ? null : parentNum + 1, + fileName, + diffPrefs.ignoreWhitespace) + : diffOperations.getModifiedFile( + notes.getProjectName(), aId, bId, fileName, diffPrefs.ignoreWhitespace); + return newBuilder().toPatchScriptNew(git, fileDiffOutput); + } + /** * The comparison is not exhaustive but is using the most important fields. Comparing all fields * will require some work in {@link PatchScript} to, e.g., convert it to autovalue. This diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java index 24c03c7547..18d532b32b 100644 --- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java +++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java @@ -163,8 +163,7 @@ public class SubmitWithStickyApprovalDiff { latestApprovedPatchsetId, currentPatchsetId, diffPreferencesInfo, - currentUser, - /* useNewDiffCache= */ false); + currentUser); PatchScript patchScript = null; try { patchScript = patchScriptFactory.call(); diff --git a/java/com/google/gerrit/server/restapi/change/GetDiff.java b/java/com/google/gerrit/server/restapi/change/GetDiff.java index af1236e353..b8902b78ab 100644 --- a/java/com/google/gerrit/server/restapi/change/GetDiff.java +++ b/java/com/google/gerrit/server/restapi/change/GetDiff.java @@ -91,10 +91,6 @@ public class GetDiff implements RestReadView { @Option(name = "--intraline") boolean intraline; - // TODO(ghareeb): This is a temporary parameter for debugging. Please remove. - @Option(name = "--use-new-diff-cache") - boolean useNewDiffCache; - @Inject GetDiff( ProjectCache projectCache, @@ -143,15 +139,13 @@ public class GetDiff implements RestReadView { } psf = patchScriptFactoryFactory.create( - notes, fileName, basePatchSet.id(), pId, prefs, currentUser.get(), useNewDiffCache); + notes, fileName, basePatchSet.id(), pId, prefs, currentUser.get()); } else if (parentNum > 0) { psf = patchScriptFactoryFactory.create( - notes, fileName, parentNum - 1, pId, prefs, currentUser.get(), useNewDiffCache); + notes, fileName, parentNum - 1, pId, prefs, currentUser.get()); } else { - psf = - patchScriptFactoryFactory.create( - notes, fileName, null, pId, prefs, currentUser.get(), useNewDiffCache); + psf = patchScriptFactoryFactory.create(notes, fileName, null, pId, prefs, currentUser.get()); } try { diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java index ec59674ffc..7a2839edcc 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java @@ -76,7 +76,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { private static final String FILE_CONTENT2 = "1st line\n2nd line\n3rd line\n"; private boolean intraline; - private boolean useNewDiffCache; + private boolean useNewDiffCacheListFiles; private boolean useNewDiffCacheGetDiff; private ObjectId commit1; @@ -91,9 +91,10 @@ public class RevisionDiffIT extends AbstractDaemonTest { baseConfig.setString("cache", "diff_intraline", "timeout", "1 minute"); intraline = baseConfig.getBoolean(TEST_PARAMETER_MARKER, "intraline", false); - useNewDiffCache = baseConfig.getBoolean("cache", "diff_cache", "useNewDiffCache", false); + useNewDiffCacheListFiles = + baseConfig.getBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", false); useNewDiffCacheGetDiff = - baseConfig.getBoolean("cache", "diff_cache", "useNewDiffCache_getDiff", false); + baseConfig.getBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", false); ObjectId headCommit = testRepo.getRepository().resolve("HEAD"); commit1 = @@ -1276,7 +1277,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { public void renamedUnrelatedFileIsIgnored_ForPatchSetDiffWithRebase_WhenEquallyModifiedInBoth() throws Exception { // TODO(ghareeb): fix this test for the new diff cache implementation - assume().that(useNewDiffCache).isFalse(); + assume().that(useNewDiffCacheListFiles).isFalse(); Function contentModification = fileContent -> fileContent.replace("1st line\n", "First line\n"); @@ -1369,7 +1370,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { @Test public void filesTouchedByPatchSetsAndContainingOnlyRebaseHunksAreIgnored() throws Exception { // TODO(ghareeb): fix this test for the new diff cache implementation - assume().that(useNewDiffCache).isFalse(); + assume().that(useNewDiffCacheListFiles).isFalse(); addModifiedPatchSet( changeId, FILE_NAME, fileContent -> fileContent.replace("Line 50\n", "Line fifty\n")); @@ -2751,7 +2752,7 @@ public class RevisionDiffIT extends AbstractDaemonTest { @Test public void symlinkConvertedToRegularFileIsIdentifiedAsAdded() throws Exception { // TODO(ghareeb): fix this test for the new diff cache implementation - assume().that(useNewDiffCache).isFalse(); + assume().that(useNewDiffCacheListFiles).isFalse(); assume().that(useNewDiffCacheGetDiff).isFalse(); String target = "file.txt"; diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java index e55f432729..46954e982d 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java @@ -26,7 +26,7 @@ public class RevisionNewDiffCacheForSingleFileIT extends RevisionDiffIT { @ConfigSuite.Default public static Config newDiffCacheConfig() { Config config = new Config(); - config.setBoolean("cache", "diff_cache", "runNewDiffCacheAsync_getDiff", true); + config.setBoolean("cache", "diff_cache", "runNewDiffCache_GetDiff", true); return config; } } diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java index 4b85c30dde..ec0bcc62de 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java @@ -18,14 +18,15 @@ import com.google.gerrit.testing.ConfigSuite; import org.eclipse.jgit.lib.Config; /** - * Runs the {@link RevisionDiffIT} tests with 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. + * 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.setBoolean("cache", "diff_cache", "useNewDiffCache", true); + config.setBoolean("cache", "diff_cache", "runNewDiffCache_ListFiles", true); return config; } }