ChangeData: Don't load ChangedLines when missing in index
I50e94118 fixed PatchListCacheImpl to throw
PatchListNotAvailableException in more cases when computing the patch
list fails, which correctly resulted in storing no added/deleted
fields in the index. However, this had the unfortunate side effect of
leaving the changedLines field null in ChangeDatas returned in
searches, which in turn led to lazy-loading of the field from the
PatchListCache. For large changes that were prone to failure, this
could lead to very long search result load times.
Instead, leave a sentinel Optional.absent() in the changedLines field
to indicate that lazy loading should not be attempted. These will show
up as having unknown size in the UI, which is fine. They will also
likely fail to load the change screen, but that's not as bad as
slowing down every single search result.
Change-Id: I1aa3b9399530e3190b5dc1e73da8bf0f640ebded
diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index bc8efc6..4747455 100644
--- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -451,6 +451,12 @@
cd.setChangedLines(
added.numericValue().intValue(),
deleted.numericValue().intValue());
+ } else {
+ // No ChangedLines stored, likely due to failure during reindexing, for
+ // example due to LargeObjectException. But we know the field was
+ // requested, so update ChangeData to prevent callers from trying to
+ // lazily load it, as that would probably also fail.
+ cd.setNoChangedLines();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 322d455..8d8b70f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -409,10 +409,10 @@
out.mergeable = cd.isMergeable();
}
out.submittable = Submit.submittable(cd);
- ChangedLines changedLines = cd.changedLines();
- if (changedLines != null) {
- out.insertions = changedLines.insertions;
- out.deletions = changedLines.deletions;
+ Optional<ChangedLines> changedLines = cd.changedLines();
+ if (changedLines.isPresent()) {
+ out.insertions = changedLines.get().insertions;
+ out.deletions = changedLines.get().deletions;
}
out.subject = in.getSubject();
out.status = in.getStatus().asChangeStatus();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/IntegerRangePredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/IntegerRangePredicate.java
index 52c1201..e62a685 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/IntegerRangePredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/IntegerRangePredicate.java
@@ -31,10 +31,13 @@
}
}
- protected abstract int getValueInt(T object) throws OrmException;
+ protected abstract Integer getValueInt(T object) throws OrmException;
public boolean match(T object) throws OrmException {
- int valueInt = getValueInt(object);
+ Integer valueInt = getValueInt(object);
+ if (valueInt == null) {
+ return false;
+ }
return valueInt >= range.min && valueInt <= range.max;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java
index 074c81a..fe448c6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -19,6 +19,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
+import com.google.common.base.Optional;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
@@ -595,9 +596,8 @@
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
-
- return input.changedLines() != null
- ? input.changedLines().insertions
+ return input.changedLines().isPresent()
+ ? input.changedLines().get().insertions
: null;
}
};
@@ -609,8 +609,8 @@
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
- return input.changedLines() != null
- ? input.changedLines().deletions
+ return input.changedLines().isPresent()
+ ? input.changedLines().get().deletions
: null;
}
};
@@ -622,9 +622,9 @@
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
- ChangedLines changedLines = input.changedLines();
- return changedLines != null
- ? changedLines.insertions + changedLines.deletions
+ Optional<ChangedLines> changedLines = input.changedLines();
+ return changedLines.isPresent()
+ ? changedLines.get().insertions + changedLines.get().deletions
: null;
}
};
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AddedPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AddedPredicate.java
index 241b7fc..b3cdd6a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AddedPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AddedPredicate.java
@@ -24,7 +24,7 @@
}
@Override
- protected int getValueInt(ChangeData changeData) throws OrmException {
- return changeData.changedLines().insertions;
+ protected Integer getValueInt(ChangeData changeData) throws OrmException {
+ return ChangeField.ADDED.get(changeData, null);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index 1794ee3..da82071 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -337,7 +337,7 @@
private ChangeControl changeControl;
private List<ChangeMessage> messages;
private List<SubmitRecord> submitRecords;
- private ChangedLines changedLines;
+ private Optional<ChangedLines> changedLines;
private SubmitTypeRecord submitTypeRecord;
private Boolean mergeable;
private Set<String> hashtags;
@@ -606,32 +606,37 @@
return files.get(ps.getPatchSetId());
}
- public ChangedLines changedLines() throws OrmException {
+ private Optional<ChangedLines> computeChangedLines() throws OrmException {
+ Change c = change();
+ if (c == null) {
+ return Optional.absent();
+ }
+ PatchSet ps = currentPatchSet();
+ if (ps == null) {
+ return Optional.absent();
+ }
+ try {
+ PatchList p = patchListCache.get(c, ps);
+ return Optional.of(
+ new ChangedLines(p.getInsertions(), p.getDeletions()));
+ } catch (PatchListNotAvailableException e) {
+ return Optional.absent();
+ }
+ }
+
+ public Optional<ChangedLines> changedLines() throws OrmException {
if (changedLines == null) {
- Change c = change();
- if (c == null) {
- return null;
- }
-
- PatchSet ps = currentPatchSet();
- if (ps == null) {
- return null;
- }
-
- PatchList p;
- try {
- p = patchListCache.get(c, ps);
- } catch (PatchListNotAvailableException e) {
- return null;
- }
-
- changedLines = new ChangedLines(p.getInsertions(), p.getDeletions());
+ changedLines = computeChangedLines();
}
return changedLines;
}
public void setChangedLines(int insertions, int deletions) {
- changedLines = new ChangedLines(insertions, deletions);
+ changedLines = Optional.of(new ChangedLines(insertions, deletions));
+ }
+
+ public void setNoChangedLines() {
+ changedLines = Optional.absent();
}
public Change.Id getId() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeletedPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeletedPredicate.java
index 9bd6956..9e49269 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeletedPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeletedPredicate.java
@@ -24,7 +24,7 @@
}
@Override
- protected int getValueInt(ChangeData changeData) throws OrmException {
- return changeData.changedLines().deletions;
+ protected Integer getValueInt(ChangeData changeData) throws OrmException {
+ return ChangeField.DELETED.get(changeData, null);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeltaPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeltaPredicate.java
index 07e6590..ce33225 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeltaPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeltaPredicate.java
@@ -16,7 +16,6 @@
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.query.QueryParseException;
-import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
import com.google.gwtorm.server.OrmException;
public class DeltaPredicate extends IntegerRangeChangePredicate {
@@ -25,8 +24,7 @@
}
@Override
- protected int getValueInt(ChangeData changeData) throws OrmException {
- ChangedLines changedLines = changeData.changedLines();
- return changedLines.insertions + changedLines.deletions;
+ protected Integer getValueInt(ChangeData changeData) throws OrmException {
+ return ChangeField.DELTA.get(changeData, null);
}
}
