drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From j..@apache.org
Subject [06/12] drill git commit: DRILL-5512: Standardize error handling in ScanBatch
Date Sat, 03 Jun 2017 04:46:01 GMT
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 <progers@maprtech.com>
Authored: Mon May 15 15:00:21 2017 -0700
Committer: Jinfeng Ni <jni@apache.org>
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 {
    * <p>The cause message will be used unless {@link Builder#message(String, Object...)}
is called.
    * <p>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.
+   * <p>
+   * 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.
+   * <p>
+   * 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<RecordReader> readers,
-                   List<Map<String, String>> implicitColumns) throws ExecutionSetupException
{
+                   List<Map<String, String>> 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 {
      * <code>FLOAT4 = 18;</code>
      *
      * <pre>
-     *  4 byte ieee 754 
+     *  4 byte ieee 754
      * </pre>
      */
     FLOAT4(17, 18),
@@ -463,7 +463,7 @@ public final class TypeProtos {
      * <code>FLOAT4 = 18;</code>
      *
      * <pre>
-     *  4 byte ieee 754 
+     *  4 byte ieee 754
      * </pre>
      */
     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 {
        *
        * <pre>
        * equivalent to SQLNonTransientException.
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assistance
        * </pre>
        */
       SYSTEM(8, 8),
@@ -2186,8 +2190,8 @@ public final class UserBitShared {
        *
        * <pre>
        * equivalent to SQLFeatureNotSupportedException
-       * - type change
-       * - schema change
+       * - unimplemented feature, option, or execution path
+       * - schema change in operator that does not support it
        * </pre>
        */
       UNSUPPORTED_OPERATION(9, 9),
@@ -2286,6 +2290,10 @@ public final class UserBitShared {
        *
        * <pre>
        * equivalent to SQLNonTransientException.
+       * - unexpected internal state
+       * - uncategorized operation
+       * general user action is to contact the Drill team for
+       * assistance
        * </pre>
        */
       public static final int SYSTEM_VALUE = 8;
@@ -2294,8 +2302,8 @@ public final class UserBitShared {
        *
        * <pre>
        * equivalent to SQLFeatureNotSupportedException
-       * - type change
-       * - schema change
+       * - unimplemented feature, option, or execution path
+       * - schema change in operator that does not support it
        * </pre>
        */
       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


Mime
View raw message