> On April 14, 2015, 3:31 p.m., Jacques Nadeau wrote:
> > 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().
>
> abdelhakim deneche wrote:
> How about the following:
> (1)
> ```
> throw UserException.wrapAsDataRead(e)
> .message(%s - %s, suffix)
> .addContext("File Name", "/myfile")
> .addContext("Line Number", 4)
> .build();
> ```
> or
>
> (2)
> ```
> throw UserException.createDataRead()
> .message(%s - %s, suffix)
> .addContext("File Name", "/myfile")
> .addContext("Line Number", 4)
> .build();
> ```
>
> This way we don't have to import the ErrorType, which most of the time
> displays as UserBitShared.DrillPBError.ErrorType by default (at least in
> IntelliJ). This way I can also make impossible for the developer to create or
> wrap inside a SystemError.
>
> Jacques Nadeau wrote:
> That is fine, although then I would be inclined to get rid of wrap versus
> create. For example
>
> throw UserException.dataRead(e)
> .message(%s - %s, suffix)
> .addContext("File Name", "/myfile")
> .addContext("Line Number", 4)
> .build();
>
> abdelhakim deneche wrote:
> I made the change, and the builder become so easy to use. Some of the
> calls can be somewhat ambiguous:
> ```
> UserException.connection()...
> UserException.function()...
> UserException.parse()...
> ```
> Is it okay, or should we change them to something like this:
> ```
> UserException.dataReadError()...
> UserException.connectionError()...
> UserException.functionError()...
> UserException.parseError()...
> ...
> ```
>
> ?
I agree. I think xxxError() is clearer and everybody has autocompletion, right?
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32987/#review80026
-----------------------------------------------------------
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
>
>
