From 977072b8ca3e26b97e077f9bd3b2a2ed52a50b02 Mon Sep 17 00:00:00 2001 From: Bruce Zu Date: Wed, 16 Apr 2014 18:34:02 +0800 Subject: [PATCH] Fix: Wrong exception mapping in ReceiveCommmits When IOException is thrown in insertPatchSet() or insertChange(), e.g. ClosedByInterruptException thrown from AutoCommitWriter.manualFlush(), it will be mapped to OrmException with error message "Error updating database" by ORM_EXCEPTION, this becomes misleading when the administrator analyzes the error log. Fixed. Replaced the ORM_EXCEPTION with INSERT_EXCEPTION. Change-Id: I49a6dfea7e49df72db775989a3a0501f60db76d2 --- .../gerrit/server/git/InsertException.java | 30 +++++++++++++++++ .../gerrit/server/git/ReceiveCommits.java | 33 +++++++++++-------- 2 files changed, 49 insertions(+), 14 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/InsertException.java diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/InsertException.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/InsertException.java new file mode 100644 index 0000000000..575ad52610 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/InsertException.java @@ -0,0 +1,30 @@ +//Copyright (C) 2014 The Android Open Source Project +// +//Licensed under the Apache License, Version 2.0 (the "License"); +//you may not use this file except in compliance with the License. +//You may obtain a copy of the License at +// +//http://www.apache.org/licenses/LICENSE-2.0 +// +//Unless required by applicable law or agreed to in writing, software +//distributed under the License is distributed on an "AS IS" BASIS, +//WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +//See the License for the specific language governing permissions and +//limitations under the License. + +package com.google.gerrit.server.git; + +/** + * Thrown in inserting change or patchset, e.g. OrmException or IOException. + */ +public class InsertException extends Exception { + private static final long serialVersionUID = 1L; + + InsertException(final String msg) { + super(msg, null); + } + + InsertException(final String msg, final Throwable why) { + super(msg, why); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 52e017bd98..94d209b8e3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -236,14 +236,17 @@ public class ReceiveCommits { } } - private static final Function ORM_EXCEPTION = - new Function() { + private static final Function INSERT_EXCEPTION = + new Function() { @Override - public OrmException apply(Exception input) { + public InsertException apply(Exception input) { if (input instanceof OrmException) { - return (OrmException) input; + return new InsertException("ORM error", input); } - return new OrmException("Error updating database", input); + if (input instanceof IOException) { + return new InsertException("IO error", input); + } + return new InsertException("Error inserting change/patchset", input); } }; @@ -647,7 +650,7 @@ public class ReceiveCommits { log.error(String.format( "Cannot add patch set to %d of %s", e.getKey().get(), project.getName()), err); - } catch (OrmException err) { + } catch (InsertException err) { reject(replace.inputCommand, "internal server error"); log.error(String.format( "Cannot add patch set to %d of %s", @@ -681,7 +684,7 @@ public class ReceiveCommits { } try { - List> futures = Lists.newArrayList(); + List> futures = Lists.newArrayList(); for (ReplaceRequest replace : replaceByChange.values()) { if (magicBranch != null && replace.inputCommand == magicBranch.cmd) { futures.add(replace.insertPatchSet()); @@ -692,12 +695,12 @@ public class ReceiveCommits { futures.add(create.insertChange()); } - for (CheckedFuture f : futures) { + for (CheckedFuture f : futures) { f.checkedGet(); } magicBranch.cmd.setResult(OK); - } catch (OrmException err) { - log.error("Can't insert changes for " + project.getName(), err); + } catch (InsertException err) { + log.error("Can't insert change/patchset for " + project.getName(), err); reject(magicBranch.cmd, "internal server error"); } catch (IOException err) { log.error("Can't read commits for " + project.getName(), err); @@ -1462,7 +1465,7 @@ public class ReceiveCommits { ins.getPatchSet().getRefName()); } - CheckedFuture insertChange() throws IOException { + CheckedFuture insertChange() throws IOException { rp.getRevWalk().parseBody(commit); final Thread caller = Thread.currentThread(); @@ -1486,7 +1489,7 @@ public class ReceiveCommits { return null; } })); - return Futures.makeChecked(future, ORM_EXCEPTION); + return Futures.makeChecked(future, INSERT_EXCEPTION); } private void insertChange(ReviewDb db) throws OrmException, IOException { @@ -1790,7 +1793,7 @@ public class ReceiveCommits { return true; } - CheckedFuture insertPatchSet() + CheckedFuture insertPatchSet() throws IOException { rp.getRevWalk().parseBody(newCommit); @@ -1817,7 +1820,7 @@ public class ReceiveCommits { } } })); - return Futures.makeChecked(future, ORM_EXCEPTION); + return Futures.makeChecked(future, INSERT_EXCEPTION); } PatchSet.Id insertPatchSet(ReviewDb db) throws OrmException, IOException { @@ -2183,6 +2186,8 @@ public class ReceiveCommits { codeReviewCommit, rw, repo, project, new ArrayList(), new HashMap()); subOp.update(); + } catch (InsertException e) { + log.error("Can't insert patchset", e); } catch (IOException e) { log.error("Can't scan for changes to close", e); } catch (OrmException e) {