----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32987/#review79537 -----------------------------------------------------------
initial comments common/src/main/java/org/apache/drill/common/exceptions/DrillDataReadException.java <https://reviews.apache.org/r/32987/#comment128949> I'm inclined not to prefix everything wiht Drill. Thoughts on this? common/src/main/java/org/apache/drill/common/exceptions/DrillParseException.java <https://reviews.apache.org/r/32987/#comment128950> purpose? Is this query parsing? Something else? If Query Parsing, should be have line and column? common/src/main/java/org/apache/drill/common/exceptions/DrillRemoteException.java <https://reviews.apache.org/r/32987/#comment128951> purpose? common/src/main/java/org/apache/drill/common/exceptions/DrillSystemException.java <https://reviews.apache.org/r/32987/#comment128952> This should be used only by the internal client layer, right? If so, let's make sure the constructors are as protected as possible. common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java <https://reviews.apache.org/r/32987/#comment128963> We probably need a push and add context. For example, I'd like to be able to add OperatorId and FragmentId to a reader error so we know what part of the plan it occurred in. That should really be prepended to the ctx. common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java <https://reviews.apache.org/r/32987/#comment128954> Can we add method like getVerboseMessage() and then choose which one we want rather than controlling on creation? common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java <https://reviews.apache.org/r/32987/#comment128956> Do we have a separate error object type for bit <> bit? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java <https://reviews.apache.org/r/32987/#comment128958> Why do we need this? exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java <https://reviews.apache.org/r/32987/#comment128959> Why do we need this? If it is a debug message, let's set it at that. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java <https://reviews.apache.org/r/32987/#comment128960> same exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java <https://reviews.apache.org/r/32987/#comment128961> This seems weird as it looks like I might get "Failure validating SQL - Sql Parsing error...". What will this actually construct? Can you add to comments here? Also, we should probably encapsulate error delineation (" - ") so that we use the same pattern everywhere. - Jacques Nadeau On April 9, 2015, 5:42 p.m., abdelhakim deneche wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32987/ > ----------------------------------------------------------- > > (Updated April 9, 2015, 5:42 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/DrillDataReadException.java > PRE-CREATION > > common/src/main/java/org/apache/drill/common/exceptions/DrillParseException.java > PRE-CREATION > > common/src/main/java/org/apache/drill/common/exceptions/DrillRemoteException.java > PRE-CREATION > > common/src/main/java/org/apache/drill/common/exceptions/DrillSystemException.java > PRE-CREATION > > common/src/main/java/org/apache/drill/common/exceptions/DrillUnsupportedOperationException.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 > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java > 18b93e9 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java > 8038527 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java > d17fdd4 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/StatusHandler.java > 5e21878 > > 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/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/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 > protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java > f72d5e1 > protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java > 9a9d196 > protocol/src/main/java/org/apache/drill/exec/proto/beans/DrillPBError.java > ac9cef5 > protocol/src/main/protobuf/UserBitShared.proto 5e44655 > > Diff: https://reviews.apache.org/r/32987/diff/ > > > Testing > ------- > > > Thanks, > > abdelhakim deneche > >
