opendev/gerrit
Code Issues Proposed changes

Merge "Switch GetDiff to use the new diff cache"

Browse Source
This commit is contained in:
Youssef Elghareeb
2021-03-18 08:45:49 +00:00
committed by Gerrit Code Review
parent adc24fd85b 02ac16d7af
commit ddc3cf20af
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 =
@@ -1302,7 +1303,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");
@@ -1395,7 +1396,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"));
@@ -2777,7 +2778,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;
}
}