From commits-return-4946-apmail-drill-commits-archive=drill.apache.org@drill.apache.org Sat Jun 3 04:45:57 2017 Return-Path: X-Original-To: apmail-drill-commits-archive@www.apache.org Delivered-To: apmail-drill-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id B091A1ACD9 for ; Sat, 3 Jun 2017 04:45:57 +0000 (UTC) Received: (qmail 75713 invoked by uid 500); 3 Jun 2017 04:45:57 -0000 Delivered-To: apmail-drill-commits-archive@drill.apache.org Received: (qmail 75628 invoked by uid 500); 3 Jun 2017 04:45:57 -0000 Mailing-List: contact commits-help@drill.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: commits@drill.apache.org Delivered-To: mailing list commits@drill.apache.org Received: (qmail 75388 invoked by uid 99); 3 Jun 2017 04:45:57 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 03 Jun 2017 04:45:57 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id EFBC3E152F; Sat, 3 Jun 2017 04:45:56 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: jni@apache.org To: commits@drill.apache.org Date: Sat, 03 Jun 2017 04:46:01 -0000 Message-Id: <57cb214ec71844ae8781f4676448a6ff@git.apache.org> In-Reply-To: References: X-Mailer: ASF-Git Admin Mailer Subject: [06/12] drill git commit: DRILL-5512: Standardize error handling in ScanBatch DRILL-5512: Standardize error handling in ScanBatch Standardizes error handling to throw a UserException. Prior code threw various exceptions, called the fail() method, or returned a variety of status codes. closes #838 Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/78739889 Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/78739889 Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/78739889 Branch: refs/heads/master Commit: 78739889164c8df84fee249310f6d72d1199ea04 Parents: 155820a Author: Paul Rogers Authored: Mon May 15 15:00:21 2017 -0700 Committer: Jinfeng Ni Committed: Fri Jun 2 21:43:14 2017 -0700 ---------------------------------------------------------------------- .../drill/common/exceptions/UserException.java | 12 +++-- .../drill/exec/physical/impl/ScanBatch.java | 51 ++++++++++---------- .../apache/drill/common/types/TypeProtos.java | 4 +- .../apache/drill/exec/proto/UserBitShared.java | 16 ++++-- protocol/src/main/protobuf/Types.proto | 7 +-- protocol/src/main/protobuf/UserBitShared.proto | 8 ++- 6 files changed, 56 insertions(+), 42 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/78739889/common/src/main/java/org/apache/drill/common/exceptions/UserException.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/drill/common/exceptions/UserException.java b/common/src/main/java/org/apache/drill/common/exceptions/UserException.java index 87b3fd4..dd4fd36 100644 --- a/common/src/main/java/org/apache/drill/common/exceptions/UserException.java +++ b/common/src/main/java/org/apache/drill/common/exceptions/UserException.java @@ -77,6 +77,14 @@ public class UserException extends DrillRuntimeException { *

The cause message will be used unless {@link Builder#message(String, Object...)} is called. *

If the wrapped exception is, or wraps, a user exception it will be returned by {@link Builder#build(Logger)} * instead of creating a new exception. Any added context will be added to the user exception as well. + *

+ * This exception, previously deprecated, has been repurposed to indicate unspecified + * errors. In particular, the case in which a lower level bit of code throws an + * exception other than UserException. The catching code then only knows "something went + * wrong", but not enough information to categorize the error. + *

+ * System errors also indicate illegal internal states, missing functionality, and other + * code-related errors -- all of which "should never occur." * * @see org.apache.drill.exec.proto.UserBitShared.DrillPBError.ErrorType#SYSTEM * @@ -84,10 +92,8 @@ public class UserException extends DrillRuntimeException { * returned by the builder instead of creating a new user exception * @return user exception builder * - * @deprecated This method should never need to be used explicitly, unless you are passing the exception to the - * Rpc layer or UserResultListener.submitFailed() */ - @Deprecated + public static Builder systemError(final Throwable cause) { return new Builder(DrillPBError.ErrorType.SYSTEM, cause); } http://git-wip-us.apache.org/repos/asf/drill/blob/78739889/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java index 5a9af39..4218069 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java @@ -86,31 +86,32 @@ public class ScanBatch implements CloseableRecordBatch { public ScanBatch(PhysicalOperator subScanConfig, FragmentContext context, OperatorContext oContext, Iterator readers, - List> implicitColumns) throws ExecutionSetupException { + List> implicitColumns) { this.context = context; this.readers = readers; if (!readers.hasNext()) { - throw new ExecutionSetupException("A scan batch must contain at least one reader."); + throw UserException.systemError( + new ExecutionSetupException("A scan batch must contain at least one reader.")) + .build(logger); } currentReader = readers.next(); this.oContext = oContext; allocator = oContext.getAllocator(); mutator = new Mutator(oContext, allocator, container); - boolean setup = false; try { oContext.getStats().startProcessing(); currentReader.setup(oContext, mutator); - setup = true; - } finally { - // if we had an exception during setup, make sure to release existing data. - if (!setup) { - try { - currentReader.close(); - } catch(final Exception e) { - throw new ExecutionSetupException(e); - } + } catch (ExecutionSetupException e) { + try { + currentReader.close(); + } catch(final Exception e2) { + logger.error("Close failed for reader " + currentReader.getClass().getSimpleName(), e2); } + throw UserException.systemError(e) + .addContext("Setup failed for", currentReader.getClass().getSimpleName()) + .build(logger); + } finally { oContext.getStats().stopProcessing(); } this.implicitColumns = implicitColumns.iterator(); @@ -173,9 +174,8 @@ public class ScanBatch implements CloseableRecordBatch { currentReader.allocate(mutator.fieldVectorMap()); } catch (OutOfMemoryException e) { - logger.debug("Caught Out of Memory Exception", e); clearFieldVectorMap(); - return IterOutcome.OUT_OF_MEMORY; + throw UserException.memoryError(e).build(logger); } while ((recordCount = currentReader.next()) == 0) { try { @@ -213,17 +213,16 @@ public class ScanBatch implements CloseableRecordBatch { try { currentReader.allocate(mutator.fieldVectorMap()); } catch (OutOfMemoryException e) { - logger.debug("Caught OutOfMemoryException"); clearFieldVectorMap(); - return IterOutcome.OUT_OF_MEMORY; + throw UserException.memoryError(e).build(logger); } addImplicitVectors(); } catch (ExecutionSetupException e) { - this.context.fail(e); releaseAssets(); - return IterOutcome.STOP; + throw UserException.systemError(e).build(logger); } } + // At this point, the current reader has read 1 or more rows. hasReadNonEmptyFile = true; @@ -245,18 +244,15 @@ public class ScanBatch implements CloseableRecordBatch { return IterOutcome.OK; } } catch (OutOfMemoryException ex) { - context.fail(UserException.memoryError(ex).build(logger)); - return IterOutcome.STOP; + throw UserException.memoryError(ex).build(logger); } catch (Exception ex) { - logger.debug("Failed to read the batch. Stopping...", ex); - context.fail(ex); - return IterOutcome.STOP; + throw UserException.systemError(ex).build(logger); } finally { oContext.getStats().stopProcessing(); } } - private void addImplicitVectors() throws ExecutionSetupException { + private void addImplicitVectors() { try { if (implicitVectors != null) { for (ValueVector v : implicitVectors.values()) { @@ -274,7 +270,10 @@ public class ScanBatch implements CloseableRecordBatch { } } } catch(SchemaChangeException e) { - throw new ExecutionSetupException(e); + // No exception should be thrown here. + throw UserException.systemError(e) + .addContext("Failure while allocating implicit vectors") + .build(logger); } } @@ -324,7 +323,7 @@ public class ScanBatch implements CloseableRecordBatch { * this scan batch. Made visible so that tests can create this mutator * without also needing a ScanBatch instance. (This class is really independent * of the ScanBatch, but resides here for historical reasons. This is, - * in turn, the only use of the genereated vector readers in the vector + * in turn, the only use of the generated vector readers in the vector * package.) */ http://git-wip-us.apache.org/repos/asf/drill/blob/78739889/protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java ---------------------------------------------------------------------- diff --git a/protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java b/protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java index ff5698a..1fa4848 100644 --- a/protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java +++ b/protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java @@ -170,7 +170,7 @@ public final class TypeProtos { * FLOAT4 = 18; * * 

-     *  4 byte ieee 754 
+     *  4 byte ieee 754
      *
*/ FLOAT4(17, 18), @@ -463,7 +463,7 @@ public final class TypeProtos { * FLOAT4 = 18; * * 
-     *  4 byte ieee 754 
+     *  4 byte ieee 754
      *
*/ public static final int FLOAT4_VALUE = 18; http://git-wip-us.apache.org/repos/asf/drill/blob/78739889/protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java ---------------------------------------------------------------------- diff --git a/protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java b/protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java index d28a13d..e4261df 100644 --- a/protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java +++ b/protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java @@ -2178,6 +2178,10 @@ public final class UserBitShared { * * 
        * equivalent to SQLNonTransientException.
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assistance
        *
*/ SYSTEM(8, 8), @@ -2186,8 +2190,8 @@ public final class UserBitShared { * * 
        * equivalent to SQLFeatureNotSupportedException
-       * - type change
-       * - schema change
+       * - unimplemented feature, option, or execution path
+       * - schema change in operator that does not support it
        *
*/ UNSUPPORTED_OPERATION(9, 9), @@ -2286,6 +2290,10 @@ public final class UserBitShared { * * 
        * equivalent to SQLNonTransientException.
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assistance
        *
*/ public static final int SYSTEM_VALUE = 8; @@ -2294,8 +2302,8 @@ public final class UserBitShared { * * 
        * equivalent to SQLFeatureNotSupportedException
-       * - type change
-       * - schema change
+       * - unimplemented feature, option, or execution path
+       * - schema change in operator that does not support it
        *
*/ public static final int UNSUPPORTED_OPERATION_VALUE = 9; http://git-wip-us.apache.org/repos/asf/drill/blob/78739889/protocol/src/main/protobuf/Types.proto ---------------------------------------------------------------------- diff --git a/protocol/src/main/protobuf/Types.proto b/protocol/src/main/protobuf/Types.proto index 71fa4ac..b2b29f0 100644 --- a/protocol/src/main/protobuf/Types.proto +++ b/protocol/src/main/protobuf/Types.proto @@ -24,7 +24,7 @@ option optimize_for = SPEED; enum MinorType { LATE = 0; // late binding type MAP = 1; // an empty map column. Useful for conceptual setup. Children listed within here - + TINYINT = 3; // single byte signed integer SMALLINT = 4; // two byte signed integer INT = 5; // four byte signed integer @@ -40,7 +40,7 @@ enum MinorType { TIMESTAMPTZ = 15; // unix epoch time in millis TIMESTAMP = 16; // TBD INTERVAL = 17; // TBD - FLOAT4 = 18; // 4 byte ieee 754 + FLOAT4 = 18; // 4 byte ieee 754 FLOAT8 = 19; // 8 byte ieee 754 BIT = 20; // single bit value (boolean) FIXEDCHAR = 21; // utf8 fixed length string, padded with spaces @@ -77,11 +77,8 @@ message MajorType { repeated MinorType sub_type = 7; // used by Union type } - - enum DataMode { OPTIONAL = 0; // nullable REQUIRED = 1; // non-nullable REPEATED = 2; // single, repeated-field } - http://git-wip-us.apache.org/repos/asf/drill/blob/78739889/protocol/src/main/protobuf/UserBitShared.proto ---------------------------------------------------------------------- diff --git a/protocol/src/main/protobuf/UserBitShared.proto b/protocol/src/main/protobuf/UserBitShared.proto index b091711..65f9698 100644 --- a/protocol/src/main/protobuf/UserBitShared.proto +++ b/protocol/src/main/protobuf/UserBitShared.proto @@ -74,11 +74,15 @@ message DrillPBError{ */ RESOURCE = 7; /* equivalent to SQLNonTransientException. + * - unexpected internal state + * - uncategorized operation + * general user action is to contact the Drill team for + * assistance */ SYSTEM = 8; /* equivalent to SQLFeatureNotSupportedException - * - type change - * - schema change + * - unimplemented feature, option, or execution path + * - schema change in operator that does not support it */ UNSUPPORTED_OPERATION = 9; /* SQL validation exception