SubmitStrategyListener: Fail for internal errors
If there is an internal error we should throw an exception so that the
user gets a '500 Internal Server Error' response, and not return a
message as part of a '409 Conflict' response that is not meaningful to
the user.
This ensures that these errors are properly counted in our error
metrics.
Throwing an exception also has the advantage that the retry and
auto-tracing logic kicks in.
The RuntimeExceptions thrown in SubmitStrategyListener are caught in
BatchUpdate#execute where they are wrapped in an UpdateException.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I2962fd872d20e6aad60536985fbc85a5446a7394
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java
index b0210ecc..b533bebc 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java
@@ -14,6 +14,9 @@
package com.google.gerrit.server.submit;
+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.requireNonNull;
+
import com.google.common.base.CharMatcher;
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
@@ -79,10 +82,7 @@
args.mergeTip.getInitialTip(),
args.mergeTip.getCurrentTip(),
alreadyMerged);
- for (Change.Id id : unmerged) {
- logger.atSevere().log("change %d: change not reachable from new branch tip", id.get());
- commitStatus.problem(id, "internal error: change not reachable from new branch tip");
- }
+ checkState(unmerged.isEmpty(), "changes not reachable from new branch tip: %s", unmerged);
}
commitStatus.maybeFailVerbose();
}
@@ -104,11 +104,9 @@
for (Change.Id id : commitStatus.getChangeIds()) {
CodeReviewCommit commit = commitStatus.get(id);
CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
- if (s == null) {
- logger.atSevere().log("change %d: change not processed by merge strategy", id.get());
- commitStatus.problem(id, "internal error: change not processed by merge strategy");
- continue;
- }
+ requireNonNull(
+ s, String.format("change %d: change not processed by merge strategy", id.get()));
+
if (commit.getStatusMessage().isPresent()) {
logger.atFine().log(
"change %d: Status for commit %s is %s. %s",
