opendev/gerrit
Code Issues Proposed changes

Switch GetDiff to use the new diff cache

Browse Source 
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
This commit is contained in:
Youssef Elghareeb
2021-03-15 17:39:53 +01:00
parent 586d48f57e
commit 02ac16d7af
7 changed files with 60 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/server/change/FileInfoJsonModule.java
View File

@@ -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);
}

93
java/com/google/gerrit/server/patch/PatchScriptFactory.java
View File

@@ -82,8 +82,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
@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<PatchScript> {
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<PatchScript> {
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<PatchScript> {
@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<PatchScript> {
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<PatchScript> {
@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<PatchScript> {
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<PatchScript> {
}
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<PatchScript> {
}
}
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<PatchScript> {
"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<PatchScript> {
});
}
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

3
java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
View File

@@ -163,8 +163,7 @@ public class SubmitWithStickyApprovalDiff {
latestApprovedPatchsetId,
currentPatchsetId,
diffPreferencesInfo,
currentUser,
/* useNewDiffCache= */ false);
currentUser);
PatchScript patchScript = null;
try {
patchScript = patchScriptFactory.call();

12
java/com/google/gerrit/server/restapi/change/GetDiff.java
View File

@@ -91,10 +91,6 @@ public class GetDiff implements RestReadView<FileResource> {
@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<FileResource> {
}
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 {

13
javatests/com/google/gerrit/acceptance/api/revision/RevisionDiffIT.java
View File

@@ -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<String, String> 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";

2
javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheForSingleFileIT.java
View File

@@ -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;
}
}

7
javatests/com/google/gerrit/acceptance/api/revision/RevisionNewDiffCacheIT.java
View File

@@ -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;
}
}