From f1ad5cb93837e8d07d9d08da7c1a48caf74bbe9f Mon Sep 17 00:00:00 2001 From: Zhihong Yu Date: Thu, 13 Feb 2014 17:26:25 +0000 Subject: [PATCH] HBASE-10452 Fix potential bugs in exception handlers (Ding Yuan) git-svn-id: https://svn.apache.org/repos/asf/hbase/trunk@1567979 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop/hbase/client/ClientScanner.java | 10 +++++---- .../org/apache/hadoop/hbase/client/Get.java | 10 +++++++-- .../org/apache/hadoop/hbase/client/Scan.java | 11 ++++++++-- .../hadoop/hbase/HBaseConfiguration.java | 7 ++++++- .../org/apache/hadoop/hbase/util/JVM.java | 3 ++- .../KeyPrefixRegionSplitPolicy.java | 6 +++++- .../regionserver/RegionMergeRequest.java | 6 ++++-- .../hbase/regionserver/SplitRequest.java | 6 ++++-- .../wal/SequenceFileLogReader.java | 21 +++++++++++++++---- 9 files changed, 61 insertions(+), 19 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java index 5e288596ad6..807975eb6c9 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java @@ -429,11 +429,13 @@ public class ClientScanner extends AbstractClientScanner { callable.setClose(); try { this.caller.callWithRetries(callable); + } catch (UnknownScannerException e) { + // We used to catch this error, interpret, and rethrow. However, we + // have since decided that it's not nice for a scanner's close to + // throw exceptions. Chances are it was just due to lease time out. } catch (IOException e) { - // We used to catch this error, interpret, and rethrow. However, we - // have since decided that it's not nice for a scanner's close to - // throw exceptions. Chances are it was just an UnknownScanner - // exception due to lease time out. + /* An exception other than UnknownScanner is unexpected. */ + LOG.warn("scanner failed to close. Exception follows: " + e); } callable = null; } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Get.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Get.java index 68c4a93019a..588b94e6ae0 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Get.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Get.java @@ -29,6 +29,8 @@ import java.util.Set; import java.util.TreeMap; import java.util.TreeSet; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.hbase.HConstants; @@ -63,6 +65,7 @@ import org.apache.hadoop.hbase.util.Bytes; @InterfaceStability.Stable public class Get extends Query implements Row, Comparable { + private static final Log LOG = LogFactory.getLog(Get.class); private byte [] row = null; private int maxVersions = 1; @@ -156,11 +159,14 @@ public class Get extends Query * @param timestamp version timestamp * @return this for invocation chaining */ - public Get setTimeStamp(long timestamp) { + public Get setTimeStamp(long timestamp) + throws IOException { try { tr = new TimeRange(timestamp, timestamp+1); } catch(IOException e) { - // Will never happen + // This should never happen, unless integer overflow or something extremely wrong... + LOG.error("TimeRange failed, likely caused by integer overflow. ", e); + throw e; } return this; } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java index 487b86a64a0..80430e6a04e 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java @@ -19,6 +19,8 @@ package org.apache.hadoop.hbase.client; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; import org.apache.hadoop.hbase.HConstants; @@ -82,6 +84,8 @@ import java.util.TreeSet; @InterfaceAudience.Public @InterfaceStability.Stable public class Scan extends Query { + private static final Log LOG = LogFactory.getLog(Scan.class); + private static final String RAW_ATTR = "_raw_"; private static final String ISOLATION_LEVEL = "_isolationlevel_"; @@ -297,11 +301,14 @@ public class Scan extends Query { * @see #setMaxVersions(int) * @return this */ - public Scan setTimeStamp(long timestamp) { + public Scan setTimeStamp(long timestamp) + throws IOException { try { tr = new TimeRange(timestamp, timestamp+1); } catch(IOException e) { - // Will never happen + // This should never happen, unless integer overflow or something extremely wrong... + LOG.error("TimeRange failed, likely caused by integer overflow. ", e); + throw e; } return this; } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java index a436901ec8d..2841fc9f96a 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java @@ -145,7 +145,12 @@ public class HBaseConfiguration extends Configuration { if (Class.forName("org.apache.hadoop.conf.ConfServlet") != null) { isShowConf = true; } - } catch (Exception e) { + } catch (LinkageError e) { + // should we handle it more aggressively in addition to log the error? + LOG.warn("Error thrown: ", e); + } catch (ClassNotFoundException ce) { + LOG.debug("ClassNotFound: ConfServlet"); + // ignore } return isShowConf; } diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/JVM.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/JVM.java index 647a0ebf206..b66104536a3 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/util/JVM.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/util/JVM.java @@ -207,7 +207,8 @@ public class JVM { if (input != null){ try { input.close(); - } catch (IOException ignored) { + } catch (IOException e) { + LOG.warn("Not able to close the InputStream", e); } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyPrefixRegionSplitPolicy.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyPrefixRegionSplitPolicy.java index 530b2359efd..df851071604 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyPrefixRegionSplitPolicy.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyPrefixRegionSplitPolicy.java @@ -62,7 +62,11 @@ public class KeyPrefixRegionSplitPolicy extends IncreasingToUpperBoundRegionSpli try { prefixLength = Integer.parseInt(prefixLengthString); } catch (NumberFormatException nfe) { - // ignore + /* Differentiate NumberFormatException from an invalid value range reported below. */ + LOG.error("Number format exception when parsing " + PREFIX_LENGTH_KEY + " for table " + + region.getTableDesc().getTableName() + ":" + + prefixLengthString + ". " + nfe); + return; } if (prefixLength <= 0) { LOG.error("Invalid value for " + PREFIX_LENGTH_KEY + " for table " diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionMergeRequest.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionMergeRequest.java index 892535551db..b51f8e15d04 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionMergeRequest.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionMergeRequest.java @@ -131,8 +131,10 @@ class RegionMergeRequest implements Runnable { try { this.tableLock.release(); } catch (IOException ex) { - LOG.warn("Could not release the table lock", ex); - //TODO: if we get here, and not abort RS, this lock will never be released + LOG.error("Could not release the table lock (something is really wrong). " + + "Aborting this server to avoid holding the lock forever."); + this.server.abort("Abort; we got an error when releasing the table lock " + + "on " + region_a.getRegionNameAsString()); } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitRequest.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitRequest.java index 6ac80f293d7..38e877120d7 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitRequest.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SplitRequest.java @@ -132,8 +132,10 @@ class SplitRequest implements Runnable { try { this.tableLock.release(); } catch (IOException ex) { - LOG.warn("Could not release the table lock", ex); - //TODO: if we get here, and not abort RS, this lock will never be released + LOG.error("Could not release the table lock (something is really wrong). " + + "Aborting this server to avoid holding the lock forever."); + this.server.abort("Abort; we got an error when releasing the table lock " + + "on " + parent.getRegionNameAsString()); } } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java index 03543fd2bdd..9fa6f98c29c 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java @@ -255,8 +255,15 @@ public class SequenceFileLogReader extends ReaderBase { Field fEnd = SequenceFile.Reader.class.getDeclaredField("end"); fEnd.setAccessible(true); end = fEnd.getLong(this.reader); - } catch(Exception e) { /* reflection fail. keep going */ } - + } catch(NoSuchFieldException nfe) { + /* reflection failure, keep going */ + } catch(IllegalAccessException iae) { + /* reflection failure, keep going */ + } catch(Exception e) { + /* All other cases. Should we handle it more aggressively? */ + LOG.warn("Unexpected exception when accessing the end field", e); + } + String msg = (this.path == null? "": this.path.toString()) + ", entryStart=" + entryStart + ", pos=" + pos + ((end == Long.MAX_VALUE) ? "" : ", end=" + end) + @@ -268,8 +275,14 @@ public class SequenceFileLogReader extends ReaderBase { .getConstructor(String.class) .newInstance(msg) .initCause(ioe); - } catch(Exception e) { /* reflection fail. keep going */ } - + } catch(NoSuchMethodException nfe) { + /* reflection failure, keep going */ + } catch(IllegalAccessException iae) { + /* reflection failure, keep going */ + } catch(Exception e) { + /* All other cases. Should we handle it more aggressively? */ + LOG.warn("Unexpected exception when accessing the end field", e); + } return ioe; } }