-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32987/#review80026
-----------------------------------------------------------
General comment on improving pattern. I propose the calling pattern to be as
follows:
(1)
throw UserException.wrap(e)
.type(ErrorType.DATA_READ)
.message(%s - %s, suffix)
.addContext("File Name", "/myfile")
.addContext("Line Number", 4)
.build();
or
(2)
throw UserException.create()
.type(ErrorType.DATA_READ)
.message(%s - %s, suffix)
.addContext("File Name", "/myfile")
.addContext("Line Number", 4)
.build();
For (1), this will generate a new message if the exception tree doesn't have a
UserException. If it does have a UserException, the type and message will be
dropped but the context will be added. For (2), we're creating a new exception
explicitly because we don't have a causing exception to pass in.
Do you think this will cover the key cases?
I think we don't need "Drill" as part of UserException. I also think that all
constructors for UserException and its children should be hidden and all entry
should be through create() or wrap().
common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java
<https://reviews.apache.org/r/32987/#comment129781>
common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java
<https://reviews.apache.org/r/32987/#comment129778>
common/src/main/java/org/apache/drill/common/exceptions/UserExceptionContext.java
<https://reviews.apache.org/r/32987/#comment129777>
comment out of date since we can no longer specify verbose properties
common/src/main/java/org/apache/drill/common/exceptions/UserExceptionContext.java
<https://reviews.apache.org/r/32987/#comment129779>
says int but is long.
- Jacques Nadeau
On April 13, 2015, 5:55 p.m., abdelhakim deneche wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32987/
> -----------------------------------------------------------
>
> (Updated April 13, 2015, 5:55 p.m.)
>
>
> Review request for drill, Jacques Nadeau, Jason Altekruse, and Parth Chandra.
>
>
> Bugs: DRILL-2675
> https://issues.apache.org/jira/browse/DRILL-2675
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> **INITIAL PATCH** with a working solution. This patch cleans the path for
> errors, especially user errors with meaningful messages, to be propagated
> properly to the client.
>
> The patch includes changes to 2 existing use cases where the error message
> was successfully improved.
>
> The general idea is: if a code wants to throw an exception that contains a
> meaningful error message, it throws a DrillUserException. The propagation
> code will make sure this exception is propagated to the client. The user
> exception object doesn't contain the final error message, but enough
> information about the error, the client will use this information to display
> a better error message.
> Any exception that is not a DrillUserException (or one of it's subclasses)
> will be considered as a system exception. For those exceptions the client
> will only display the error id and drillbit identity in case the user wants
> to check the logs for more informations about the error.
> Error objects sent to the client will still contain a stack trace that can be
> used to display more information if the client has enabled the error verbose
> mode.
>
>
> Diffs
> -----
>
>
> common/src/main/java/org/apache/drill/common/exceptions/DrillRemoteException.java
> PRE-CREATION
>
> common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java
> PRE-CREATION
> common/src/main/java/org/apache/drill/common/exceptions/ErrorHelper.java
> PRE-CREATION
>
> common/src/main/java/org/apache/drill/common/exceptions/UserExceptionContext.java
> PRE-CREATION
>
> common/src/test/java/org/apache/drill/common/exceptions/TestDrillUserException.java
> PRE-CREATION
> exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java
> 9a948fb
>
> exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java
> 98948af
> exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java
> da2229c
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
> 6b3caf4
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java
> b98778d
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/CoordinationQueue.java
> 0016d6a
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RemoteRpcException.java
> 14ea873
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java b974963
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java
> a1be83b
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java
> 934a094
>
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java
> fbbf0b8
>
> exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
> cc7cb83
> exec/java-exec/src/main/java/org/apache/drill/exec/work/ErrorHelper.java
> 0773d6c
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java
> 23ef0d3
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
> 8626d5b
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/SqlUnsupportedException.java
> 2299afa
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/AbstractStatusReporter.java
> 1b0885d
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java
> a7e6c46
>
> exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/StatusReporter.java
> 26b5d68
> exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 0c2f0e5
> exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java
> 5703bf9
> exec/java-exec/src/test/java/org/apache/drill/TestBugFixes.java 875fb25
>
> exec/java-exec/src/test/java/org/apache/drill/TestDisabledFunctionality.java
> f62f060
> exec/java-exec/src/test/java/org/apache/drill/TestStarQueries.java effef9b
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 11d83f9
>
> exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java
> e03098a
>
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java
> 55f0d75
>
> exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetPhysicalPlan.java
> 882cdbd
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java 3b38a09
> exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 74900bc
> protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java
> f72d5e1
> protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java
> ac1bcbb
> protocol/src/main/java/org/apache/drill/exec/proto/beans/DrillPBError.java
> ac9cef5
> protocol/src/main/protobuf/UserBitShared.proto 2938114
>
> Diff: https://reviews.apache.org/r/32987/diff/
>
>
> Testing
> -------
>
> Unit tests are passing. Regression, customer and tpch100 are successful
>
>
> Thanks,
>
> abdelhakim deneche
>
>
