Compute only as many owned paths as needed, not all
When a code owner approval is applied and when a code owner is added as
a reviewer we post a change message that lists the owned paths. To avoid
that the change messages get too large there is a configurable limit for
the maximum number of paths that should be shown in change messages. If
a user owns more paths the change message includes "(X more files)" to
let the user know about this. For being able to post this message we are
computing all paths of the change that are owned by the user.
Computing owned path is rather expensive and if a change touches many
files this increases the latency of the post review call. To improve the
latency we are now limiting the computation of the owned paths to the
number of paths that we are going to include into the change message, +1
to know if there are more owned files. This means that the change
message can no longer say how many more owned files there are, but
improving the latency seems more important than keeping this number
included. If there are more owned files we will now only say "(more
files)" to let the user know about this.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Id33722dc09144710758db5c84de4772656b9a4b5
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 8361d9b..1c5e87f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -122,30 +122,39 @@
* @param changeNotes the change notes for which the owned files should be returned
* @param patchSet the patch set for which the owned files should be returned
* @param accountId account ID of the code owner for which the owned files should be returned
+ * @param limit the max number of owned paths that should be returned (0 = unlimited)
* @return the paths of the files in the given patch set that are owned by the specified account
* @throws ResourceConflictException if the destination branch of the change no longer exists
*/
public ImmutableList<Path> getOwnedPaths(
- ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
+ ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int limit)
throws ResourceConflictException {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
logger.atFine().log(
- "compute owned paths for account %d (project = %s, change = %d, patch set = %d)",
+ "compute owned paths for account %d (project = %s, change = %d, patch set = %d,"
+ + " limit = %d)",
accountId.get(),
changeNotes.getProjectName(),
changeNotes.getChangeId().get(),
- patchSet.id().get());
- return getFileStatusesForAccount(changeNotes, patchSet, accountId)
- .flatMap(
- fileCodeOwnerStatus ->
- Stream.of(
- fileCodeOwnerStatus.newPathStatus(), fileCodeOwnerStatus.oldPathStatus())
- .filter(Optional::isPresent)
- .map(Optional::get))
- .filter(pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
- .map(PathCodeOwnerStatus::path)
- .sorted(comparing(Path::toString))
- .collect(toImmutableList());
+ patchSet.id().get(),
+ limit);
+ Stream<Path> ownedPaths =
+ getFileStatusesForAccount(changeNotes, patchSet, accountId)
+ .flatMap(
+ fileCodeOwnerStatus ->
+ Stream.of(
+ fileCodeOwnerStatus.newPathStatus(),
+ fileCodeOwnerStatus.oldPathStatus())
+ .filter(Optional::isPresent)
+ .map(Optional::get))
+ .filter(
+ pathCodeOwnerStatus -> pathCodeOwnerStatus.status() == CodeOwnerStatus.APPROVED)
+ .map(PathCodeOwnerStatus::path)
+ .sorted(comparing(Path::toString));
+ if (limit > 0) {
+ ownedPaths = ownedPaths.limit(limit);
+ }
+ return ownedPaths.collect(toImmutableList());
} catch (IOException | PatchListNotAvailableException e) {
throw new CodeOwnersInternalServerErrorException(
String.format(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 9352e97..e6a55ee 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -96,8 +96,8 @@
CodeOwnersPluginConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(projectName);
- if (codeOwnersConfig.isDisabled(event.getChange().branch)
- || codeOwnersConfig.getMaxPathsInChangeMessages() <= 0) {
+ int maxPathsInChangeMessages = codeOwnersConfig.getMaxPathsInChangeMessages();
+ if (codeOwnersConfig.isDisabled(event.getChange().branch) || maxPathsInChangeMessages <= 0) {
return;
}
@@ -108,7 +108,8 @@
updateFactory -> {
try (BatchUpdate batchUpdate =
updateFactory.create(projectName, userProvider.get(), TimeUtil.nowTs())) {
- batchUpdate.addOp(changeId, new Op(event.getReviewers()));
+ batchUpdate.addOp(
+ changeId, new Op(event.getReviewers(), maxPathsInChangeMessages));
batchUpdate.execute();
}
return null;
@@ -124,9 +125,11 @@
private class Op implements BatchUpdateOp {
private final List<AccountInfo> reviewers;
+ private final int limit;
- Op(List<AccountInfo> reviewers) {
+ Op(List<AccountInfo> reviewers, int limit) {
this.reviewers = reviewers;
+ this.limit = limit;
}
@Override
@@ -158,9 +161,10 @@
ImmutableList<Path> ownedPaths;
try {
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), reviewerAccountId);
+ changeNotes, changeNotes.getCurrentPatchSet(), reviewerAccountId, limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
@@ -181,15 +185,11 @@
"%s who was added as reviewer owns the following files:\n",
reviewerAccount.getName()));
- int maxPathsInChangeMessage =
- codeOwnersPluginConfiguration.getProjectConfig(projectName).getMaxPathsInChangeMessages();
- if (ownedPaths.size() <= maxPathsInChangeMessage) {
+ if (ownedPaths.size() <= limit) {
appendPaths(message, ownedPaths.stream());
} else {
- // -1 so that we never show "(1 more files)"
- int limit = maxPathsInChangeMessage - 1;
appendPaths(message, ownedPaths.stream().limit(limit));
- message.append(String.format("(%s more files)\n", ownedPaths.size() - limit));
+ message.append("(more files)\n");
}
return Optional.of(message.toString());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 101cedd..e9e2c39 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -77,7 +77,9 @@
Map<String, Short> approvals) {
CodeOwnersPluginConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
- if (codeOwnersConfig.isDisabled(changeNotes.getChange().getDest().branch())) {
+ int maxPathsInChangeMessage = codeOwnersConfig.getMaxPathsInChangeMessages();
+ if (codeOwnersConfig.isDisabled(changeNotes.getChange().getDest().branch())
+ || maxPathsInChangeMessage <= 0) {
return Optional.empty();
}
@@ -97,7 +99,13 @@
try (Timer0.Context ctx = codeOwnerMetrics.extendChangeMessageOnPostReview.start()) {
return buildMessageForCodeOwnerApproval(
- user, changeNotes, patchSet, oldApprovals, approvals, requiredApproval);
+ user,
+ changeNotes,
+ patchSet,
+ oldApprovals,
+ approvals,
+ requiredApproval,
+ maxPathsInChangeMessage);
}
}
@@ -107,21 +115,16 @@
PatchSet patchSet,
Map<String, Short> oldApprovals,
Map<String, Short> approvals,
- RequiredApproval requiredApproval) {
- CodeOwnersPluginConfigSnapshot codeOwnersConfig =
- codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
- int maxPathsInChangeMessage = codeOwnersConfig.getMaxPathsInChangeMessages();
- if (maxPathsInChangeMessage <= 0) {
- return Optional.empty();
- }
-
+ RequiredApproval requiredApproval,
+ int limit) {
LabelVote newVote = getNewVote(requiredApproval, approvals);
ImmutableList<Path> ownedPaths;
try {
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes, changeNotes.getCurrentPatchSet(), user.getAccountId());
+ changeNotes, changeNotes.getCurrentPatchSet(), user.getAccountId(), limit + 1);
} catch (RestApiException e) {
logger.atFine().withCause(e).log(
"Couldn't compute owned paths of change %s for account %s",
@@ -147,7 +150,9 @@
}
boolean hasImplicitApprovalByUser =
- codeOwnersConfig.areImplicitApprovalsEnabled()
+ codeOwnersPluginConfiguration
+ .getProjectConfig(changeNotes.getProjectName())
+ .areImplicitApprovalsEnabled()
&& patchSet.uploader().equals(user.getAccountId());
boolean noLongerExplicitlyApproved = false;
@@ -213,13 +218,11 @@
return Optional.empty();
}
- if (ownedPaths.size() <= maxPathsInChangeMessage) {
+ if (ownedPaths.size() <= limit) {
appendPaths(message, ownedPaths.stream());
} else {
- // -1 so that we never show "(1 more files)"
- int limit = maxPathsInChangeMessage - 1;
appendPaths(message, ownedPaths.stream().limit(limit));
- message.append(String.format("(%s more files)\n", ownedPaths.size() - limit));
+ message.append("(more files)\n");
}
if (hasImplicitApprovalByUser && noLongerExplicitlyApproved) {
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
index 305b31a..0430f94 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetOwnedPaths.java
@@ -66,7 +66,7 @@
ImmutableList<Path> ownedPaths =
codeOwnerApprovalCheck.getOwnedPaths(
- revisionResource.getNotes(), revisionResource.getPatchSet(), accountId);
+ revisionResource.getNotes(), revisionResource.getPatchSet(), accountId, /* limit= */ 0);
OwnedPathsInfo ownedPathsInfo = new OwnedPathsInfo();
ownedPathsInfo.ownedPaths = ownedPaths.stream().map(Path::toString).collect(toImmutableList());
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
index 576d2fe..91c40db 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
@@ -210,6 +210,7 @@
String path2 = "foo/baz.bar";
String path3 = "bar/foo.baz";
String path4 = "bar/baz.foo";
+ String path5 = "baz/foo.bar";
String changeId =
createChange(
"Test Change",
@@ -221,6 +222,8 @@
path3,
"file content",
path4,
+ "file content",
+ path5,
"file content"))
.getChangeId();
@@ -233,8 +236,9 @@
"%s who was added as reviewer owns the following files:\n"
+ "* %s\n"
+ "* %s\n"
- + "(2 more files)\n",
- user.fullName(), path4, path3));
+ + "* %s\n"
+ + "(more files)\n",
+ user.fullName(), path4, path3, path5));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index 971f8e9..8a8b9c4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -808,6 +808,7 @@
String path2 = "foo/baz.bar";
String path3 = "bar/foo.baz";
String path4 = "bar/baz.foo";
+ String path5 = "baz/foo.bar";
String changeId =
createChange(
"Test Change",
@@ -819,6 +820,8 @@
path3,
"file content",
path4,
+ "file content",
+ path5,
"file content"))
.getChangeId();
@@ -833,8 +836,9 @@
+ " %s:\n"
+ "* %s\n"
+ "* %s\n"
- + "(2 more files)\n",
- admin.fullName(), path4, path3));
+ + "* %s\n"
+ + "(more files)\n",
+ admin.fullName(), path4, path3, path5));
}
@Test
