From 5aa3631e06170cb13cd1b4261f8e0b1b45d3074f Mon Sep 17 00:00:00 2001 From: Paul Rogers Date: Mon, 15 May 2017 15:00:21 -0700 Subject: [PATCH 1/3] 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. --- .../drill/exec/physical/impl/ScanBatch.java | 49 +++++++++---------- 1 file changed, 24 insertions(+), 25 deletions(-) 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 5a9af395385..ba91a3a19ce 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.executionError( + 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.executionError(e) + .addContext("Setup failed for", currentReader.getClass().getSimpleName()) + .build(logger); + } finally { oContext.getStats().stopProcessing(); } this.implicitColumns = implicitColumns.iterator(); @@ -173,9 +174,8 @@ public IterOutcome next() { 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 IterOutcome next() { 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.executionError(e).build(logger); } } + // At this point, the current reader has read 1 or more rows. hasReadNonEmptyFile = true; @@ -245,18 +244,15 @@ public IterOutcome next() { 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.unspecifiedError(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 @@ private void addImplicitVectors() throws ExecutionSetupException { } } } catch(SchemaChangeException e) { - throw new ExecutionSetupException(e); + // No exception should be thrown here. + throw UserException.internalError(e) + .addContext("Failure while allocating implicit vectors") + .build(logger); } } From cc377045682e30a2a188d3631c7721cf8e6deda6 Mon Sep 17 00:00:00 2001 From: Paul Rogers Date: Tue, 16 May 2017 11:52:19 -0700 Subject: [PATCH 2/3] Revisions based on code review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Backed out UserError changes. Repurposed existing categories per Parth’s suggestions. 2. Revised ScanBatch to use the existing categories. The result is that this PR is now branched directly from master. --- .../common/exceptions/UserException.java | 12 ++++++++--- .../drill/exec/physical/impl/ScanBatch.java | 12 +++++------ .../apache/drill/common/types/TypeProtos.java | 4 ++-- .../drill/exec/proto/UserBitShared.java | 20 +++++++++++++++---- protocol/src/main/protobuf/Types.proto | 7 ++----- .../src/main/protobuf/UserBitShared.proto | 10 ++++++++-- 6 files changed, 43 insertions(+), 22 deletions(-) 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 87b3fd4e109..dd4fd36b887 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 static Builder memoryError() { *

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 static Builder memoryError() { * 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); } 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 ba91a3a19ce..42180699afa 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 @@ -90,7 +90,7 @@ public ScanBatch(PhysicalOperator subScanConfig, FragmentContext context, this.context = context; this.readers = readers; if (!readers.hasNext()) { - throw UserException.executionError( + throw UserException.systemError( new ExecutionSetupException("A scan batch must contain at least one reader.")) .build(logger); } @@ -108,7 +108,7 @@ public ScanBatch(PhysicalOperator subScanConfig, FragmentContext context, } catch(final Exception e2) { logger.error("Close failed for reader " + currentReader.getClass().getSimpleName(), e2); } - throw UserException.executionError(e) + throw UserException.systemError(e) .addContext("Setup failed for", currentReader.getClass().getSimpleName()) .build(logger); } finally { @@ -219,7 +219,7 @@ public IterOutcome next() { addImplicitVectors(); } catch (ExecutionSetupException e) { releaseAssets(); - throw UserException.executionError(e).build(logger); + throw UserException.systemError(e).build(logger); } } @@ -246,7 +246,7 @@ public IterOutcome next() { } catch (OutOfMemoryException ex) { throw UserException.memoryError(ex).build(logger); } catch (Exception ex) { - throw UserException.unspecifiedError(ex).build(logger); + throw UserException.systemError(ex).build(logger); } finally { oContext.getStats().stopProcessing(); } @@ -271,7 +271,7 @@ private void addImplicitVectors() { } } catch(SchemaChangeException e) { // No exception should be thrown here. - throw UserException.internalError(e) + throw UserException.systemError(e) .addContext("Failure while allocating implicit vectors") .build(logger); } @@ -323,7 +323,7 @@ public VectorWrapper getValueAccessorById(Class clazz, int... ids) { * 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.) */ 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 ff5698a9187..1fa4848de74 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 enum MinorType * FLOAT4 = 18; * * 

-     *  4 byte ieee 754 
+     *  4 byte ieee 754
      *
*/ FLOAT4(17, 18), @@ -463,7 +463,7 @@ public enum MinorType * FLOAT4 = 18; * * 
-     *  4 byte ieee 754 
+     *  4 byte ieee 754
      *
*/ public static final int FLOAT4_VALUE = 18; 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 d28a13d6d39..89c69404841 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,8 @@ public enum ErrorType * * 
        * equivalent to SQLNonTransientException.
+       * - out of memory
+       * - out of disk space
        *
*/ SYSTEM(8, 8), @@ -2186,8 +2188,11 @@ public enum ErrorType * * 
        * equivalent to SQLFeatureNotSupportedException
-       * - type change
-       * - schema change
+       * - unsupported operation
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assiatance
        *
*/ UNSUPPORTED_OPERATION(9, 9), @@ -2198,6 +2203,7 @@ public enum ErrorType * SQL validation exception * - invalid schema path * - invalid entries in SQL tree + * - unimplemented feature, option, or execution path * */ VALIDATION(10, 10), @@ -2286,6 +2292,8 @@ public enum ErrorType * * 
        * equivalent to SQLNonTransientException.
+       * - out of memory
+       * - out of disk space
        *
*/ public static final int SYSTEM_VALUE = 8; @@ -2294,8 +2302,11 @@ public enum ErrorType * * 
        * equivalent to SQLFeatureNotSupportedException
-       * - type change
-       * - schema change
+       * - unsupported operation
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assiatance
        *
*/ public static final int UNSUPPORTED_OPERATION_VALUE = 9; @@ -2306,6 +2317,7 @@ public enum ErrorType * SQL validation exception * - invalid schema path * - invalid entries in SQL tree + * - unimplemented feature, option, or execution path * */ public static final int VALIDATION_VALUE = 10; diff --git a/protocol/src/main/protobuf/Types.proto b/protocol/src/main/protobuf/Types.proto index 71fa4acd9c8..b2b29f08502 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 } - diff --git a/protocol/src/main/protobuf/UserBitShared.proto b/protocol/src/main/protobuf/UserBitShared.proto index b09171146a8..a52cf0a45a7 100644 --- a/protocol/src/main/protobuf/UserBitShared.proto +++ b/protocol/src/main/protobuf/UserBitShared.proto @@ -74,16 +74,22 @@ message DrillPBError{ */ RESOURCE = 7; /* equivalent to SQLNonTransientException. + * - out of memory + * - out of disk space */ SYSTEM = 8; /* equivalent to SQLFeatureNotSupportedException - * - type change - * - schema change + * - unsupported operation + * - unexpected internal state + * - uncategorized operation + * general user action is to contact the Drill team for + * assiatance */ UNSUPPORTED_OPERATION = 9; /* SQL validation exception * - invalid schema path * - invalid entries in SQL tree + * - unimplemented feature, option, or execution path */ VALIDATION = 10; } From 5a1d3f95c7b677fdfd4a3a694c5fd3b801d51480 Mon Sep 17 00:00:00 2001 From: Paul Rogers Date: Thu, 18 May 2017 16:39:40 -0700 Subject: [PATCH 3/3] Revised comments based on code review --- .../drill/exec/proto/UserBitShared.java | 28 ++++++++----------- .../src/main/protobuf/UserBitShared.proto | 14 ++++------ 2 files changed, 18 insertions(+), 24 deletions(-) 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 89c69404841..e4261df955d 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,8 +2178,10 @@ public enum ErrorType * * 
        * equivalent to SQLNonTransientException.
-       * - out of memory
-       * - out of disk space
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assistance
        *
*/ SYSTEM(8, 8), @@ -2188,11 +2190,8 @@ public enum ErrorType * * 
        * equivalent to SQLFeatureNotSupportedException
-       * - unsupported operation
-       * - unexpected internal state
-       * - uncategorized operation
-       * general user action is to contact the Drill team for
-       * assiatance
+       * - unimplemented feature, option, or execution path
+       * - schema change in operator that does not support it
        *
*/ UNSUPPORTED_OPERATION(9, 9), @@ -2203,7 +2202,6 @@ public enum ErrorType * SQL validation exception * - invalid schema path * - invalid entries in SQL tree - * - unimplemented feature, option, or execution path * */ VALIDATION(10, 10), @@ -2292,8 +2290,10 @@ public enum ErrorType * * 
        * equivalent to SQLNonTransientException.
-       * - out of memory
-       * - out of disk space
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assistance
        *
*/ public static final int SYSTEM_VALUE = 8; @@ -2302,11 +2302,8 @@ public enum ErrorType * * 
        * equivalent to SQLFeatureNotSupportedException
-       * - unsupported operation
-       * - unexpected internal state
-       * - uncategorized operation
-       * general user action is to contact the Drill team for
-       * assiatance
+       * - unimplemented feature, option, or execution path
+       * - schema change in operator that does not support it
        *
*/ public static final int UNSUPPORTED_OPERATION_VALUE = 9; @@ -2317,7 +2314,6 @@ public enum ErrorType * SQL validation exception * - invalid schema path * - invalid entries in SQL tree - * - unimplemented feature, option, or execution path * */ public static final int VALIDATION_VALUE = 10; diff --git a/protocol/src/main/protobuf/UserBitShared.proto b/protocol/src/main/protobuf/UserBitShared.proto index a52cf0a45a7..65f9698c1a4 100644 --- a/protocol/src/main/protobuf/UserBitShared.proto +++ b/protocol/src/main/protobuf/UserBitShared.proto @@ -74,22 +74,20 @@ message DrillPBError{ */ RESOURCE = 7; /* equivalent to SQLNonTransientException. - * - out of memory - * - out of disk space - */ - SYSTEM = 8; - /* equivalent to SQLFeatureNotSupportedException - * - unsupported operation * - unexpected internal state * - uncategorized operation * general user action is to contact the Drill team for - * assiatance + * assistance + */ + SYSTEM = 8; + /* equivalent to SQLFeatureNotSupportedException + * - unimplemented feature, option, or execution path + * - schema change in operator that does not support it */ UNSUPPORTED_OPERATION = 9; /* SQL validation exception * - invalid schema path * - invalid entries in SQL tree - * - unimplemented feature, option, or execution path */ VALIDATION = 10; }