opendev/gerrit
Code Issues Proposed changes

ChangeNotes: Catch all exceptions while scanning changes

Browse Source 
ChangeNotes has a special function for scanning over all changes in
a repo. This is intended to be used during migrations and batch index
operations. It has error handling built-in in that when ChangeNotes for
a change are not parsable, it will return a loggable error instead.
But it won't crash, and we don't want it to do that because the
batch operation should carry on.

This commit makes this logic catch all exceptions, not just
StorageExceptions to be more true to this design. We've encountered
an issue with a JSON parse exception on googlesource.com that makes
the current logic fall over. This commit fixes that.

Change-Id: Ic833f3fba285d205a5069c2dd1f6d6fed8285fd7
This commit is contained in:
Patrick Hiesel
2020-08-27 14:44:43 +02:00
parent c8a089da14
commit ae59575df4
2 changed files with 4 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/google/gerrit/server/index/change/AllChangesIndexer.java
View File

@@ -311,7 +311,7 @@ public class AllChangesIndexer extends SiteIndexer<Change.Id, ChangeData, Change
}
}
private void fail(String error, boolean failed, Exception e) {
private void fail(String error, boolean failed, Throwable e) {
if (failed) {
this.failed.update(1);
}

7
java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -51,7 +51,6 @@ import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.RobotComment;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.server.AssigneeStatusUpdate;
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
@@ -248,7 +247,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
ChangeNotes n = new ChangeNotes(args, rawChangeFromNoteDb, true, null);
try {
n.load();
} catch (StorageException e) {
} catch (Exception e) {
return ChangeNotesResult.error(n.getChangeId(), e);
}
return ChangeNotesResult.notes(n);
@@ -257,7 +256,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
/** Result of {@link #scan(Repository,Project.NameKey)}. */
@AutoValue
public abstract static class ChangeNotesResult {
static ChangeNotesResult error(Change.Id id, StorageException e) {
static ChangeNotesResult error(Change.Id id, Throwable e) {
return new AutoValue_ChangeNotes_Factory_ChangeNotesResult(id, Optional.of(e), null);
}
@@ -270,7 +269,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
public abstract Change.Id id();
/** Error encountered while loading this change, if any. */
public abstract Optional<StorageException> error();
public abstract Optional<Throwable> error();
/**
* Notes loaded for this change.