> 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().
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. - abdelhakim ----------------------------------------------------------- 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 > >