GET files?reviewed: Don't fire N+1 selects for N patch sets
If current patch set doesn't have any reviewed bit set on any files set
by current user, the algorithm iterates over all patch sets in reversed
order, fires dedicated select statement for each patch set and copies
the reviewed bits from the latest patch set found, where reviewed bits
were set. There is off by one mistake in the current implementation, so
that the select for the current patch set is fired twice.
In the worst case the current implementation also fires N+1 select
statements for N patch set. Especially on big gerrit sites, we have seen
changes with three digits patch set number, so that we eventualy spawn
hundreds of select statements with the current approach to load only one
single change.
This is a known anti pattern in SQL DML domain, to span dynamic number
of select statements, that depends on the current input (change).
To rectify, express the desired query with SQL. For a given user, change
and specific patch set (that is not necessarily the current patch set of
the change), find a patch set with max patch set number, that is smaller
or equals to the given patch set, with reviewed bit set.
Bug: Issue 5907
Change-Id: I09d345f6a396d1c9ceea4a2135f539ea073f59cd
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java
index 1ea8404..3bb98ff 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/AccountPatchReviewStore.java
@@ -14,6 +14,9 @@
package com.google.gerrit.server.change;
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gwtorm.server.OrmException;
@@ -32,6 +35,22 @@
* masters.
*/
public interface AccountPatchReviewStore {
+
+ /**
+ * Represents patch set id with reviewed files.
+ */
+ @AutoValue
+ abstract class PatchSetWithReviewedFiles {
+ abstract PatchSet.Id patchSetId();
+ abstract ImmutableSet<String> files();
+
+ public static PatchSetWithReviewedFiles create(
+ PatchSet.Id id, ImmutableSet<String> files) {
+ return new AutoValue_AccountPatchReviewStore_PatchSetWithReviewedFiles(
+ id, files);
+ }
+ }
+
/**
* Marks the given file in the given patch set as reviewed by the given user.
*
@@ -77,17 +96,16 @@
*/
void clearReviewed(PatchSet.Id psId) throws OrmException;
-
/**
- * Returns the paths of all files in the given patch set the have been
- * reviewed by the given user.
+ * Find the latest patch set, that is smaller or equals to the given patch set,
+ * where at least, one file has been reviewed by the given user.
*
* @param psId patch set ID
* @param accountId account ID of the user
- * @return the paths of all files in the given patch set the have been
- * reviewed by the given user
+ * @return optionally, all files the have been reviewed by the given user
+ * that belong to the patch set that is smaller or equals to the given patch set
* @throws OrmException thrown if accessing the reviewed flags failed
*/
- Collection<String> findReviewed(PatchSet.Id psId, Account.Id accountId)
- throws OrmException;
+ Optional<PatchSetWithReviewedFiles> findReviewed(PatchSet.Id psId,
+ Account.Id accountId) throws OrmException;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
index 35dbec1..16f2e5f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
@@ -14,8 +14,8 @@
package com.google.gerrit.server.change;
+import com.google.common.base.Optional;
import com.google.common.collect.Lists;
-import com.google.common.collect.Sets;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -34,6 +34,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.change.AccountPatchReviewStore.PatchSetWithReviewedFiles;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
@@ -58,6 +59,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -228,35 +230,25 @@
}
Account.Id userId = user.getAccountId();
- Collection<String> r = accountPatchReviewStore.get()
- .findReviewed(resource.getPatchSet().getId(), userId);
+ PatchSet patchSetId = resource.getPatchSet();
+ Optional<PatchSetWithReviewedFiles> o = accountPatchReviewStore.get()
+ .findReviewed(patchSetId.getId(), userId);
- if (r.isEmpty() && 1 < resource.getPatchSet().getPatchSetId()) {
- for (PatchSet ps : reversePatchSets(resource)) {
- Collection<String> o =
- accountPatchReviewStore.get().findReviewed(ps.getId(), userId);
- if (!o.isEmpty()) {
- try {
- r = copy(Sets.newHashSet(o), ps.getId(), resource, userId);
- } catch (IOException | PatchListNotAvailableException e) {
- log.warn("Cannot copy patch review flags", e);
- }
- break;
- }
+ if (o.isPresent()) {
+ PatchSetWithReviewedFiles res = o.get();
+ if (res.patchSetId().equals(patchSetId.getId())) {
+ return res.files();
+ }
+
+ try {
+ return copy(res.files(), res.patchSetId(), resource,
+ userId);
+ } catch (IOException | PatchListNotAvailableException e) {
+ log.warn("Cannot copy patch review flags", e);
}
}
- return r;
- }
-
- private List<PatchSet> reversePatchSets(RevisionResource resource)
- throws OrmException {
- Collection<PatchSet> patchSets =
- psUtil.byChange(db.get(), resource.getNotes());
- List<PatchSet> list = (patchSets instanceof List) ?
- (List<PatchSet>) patchSets
- : new ArrayList<>(patchSets);
- return Lists.reverse(list);
+ return Collections.emptyList();
}
private List<String> copy(Set<String> paths, PatchSet.Id old,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java
index 797f102..b5862f2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/H2AccountPatchReviewStore.java
@@ -15,6 +15,8 @@
package com.google.gerrit.server.schema;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableSet;
import com.google.common.primitives.Ints;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.registration.DynamicItem;
@@ -40,9 +42,8 @@
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
-import java.util.ArrayList;
import java.util.Collection;
-import java.util.List;
+
import javax.sql.DataSource;
@Singleton
@@ -246,21 +247,35 @@
}
@Override
- public Collection<String> findReviewed(PatchSet.Id psId, Account.Id accountId)
- throws OrmException {
+ public Optional<PatchSetWithReviewedFiles> findReviewed(PatchSet.Id psId,
+ Account.Id accountId) throws OrmException {
try (Connection con = ds.getConnection();
PreparedStatement stmt =
- con.prepareStatement("SELECT FILE_NAME FROM account_patch_reviews "
- + "WHERE account_id = ? AND change_id = ? AND patch_set_id = ?")) {
+ con.prepareStatement(
+ "SELECT patch_set_id, file_name FROM account_patch_reviews APR1 "
+ + "WHERE account_id = ? AND change_id = ? AND patch_set_id = "
+ + "(SELECT MAX(patch_set_id) FROM account_patch_reviews APR2 WHERE "
+ + "APR1.account_id = APR2.account_id "
+ + "AND APR1.change_id = APR2.change_id "
+ + "AND patch_set_id <= ?)")) {
stmt.setInt(1, accountId.get());
stmt.setInt(2, psId.getParentKey().get());
stmt.setInt(3, psId.get());
try (ResultSet rs = stmt.executeQuery()) {
- List<String> files = new ArrayList<>();
- while (rs.next()) {
- files.add(rs.getString("FILE_NAME"));
+ if (rs.next()) {
+ PatchSet.Id id = new PatchSet.Id(psId.getParentKey(),
+ rs.getInt("PATCH_SET_ID"));
+ ImmutableSet.Builder<String> builder = ImmutableSet.builder();
+ do {
+ builder.add(rs.getString("FILE_NAME"));
+ } while (rs.next());
+
+ return Optional.of(
+ AccountPatchReviewStore.PatchSetWithReviewedFiles.create(
+ id, builder.build()));
}
- return files;
+
+ return Optional.absent();
}
} catch (SQLException e) {
throw convertError("select", e);
