> On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > common/src/main/java/org/apache/drill/common/exceptions/DrillDataReadException.java, > > line 26 > > <https://reviews.apache.org/r/32987/diff/2/?file=921681#file921681line26> > > > > I'm inclined not to prefix everything wiht Drill. Thoughts on this?
I initially went with UserException, DataReadUserException, ParseUserException, UnsupportedOperationUserException... but SystemUserException doesn't sound right and SystemException sounds like something really bad did happen > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > common/src/main/java/org/apache/drill/common/exceptions/DrillParseException.java, > > line 22 > > <https://reviews.apache.org/r/32987/diff/2/?file=921682#file921682line22> > > > > purpose? Is this query parsing? Something else? If Query Parsing, > > should be have line and column? according to the Exception Types docment: a Parsing Error can include any of the following: - typos - missing table - SQL keyword misuse - function names/resolution You can add context using addContext(String) method, for example: ``` parseException.addContext("line 2, column 18"); ``` or ``` parseException.addContext("line: 2"); parseException.addContext("column: 18"); ``` All context information will be concatenated when generating the user error message > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > common/src/main/java/org/apache/drill/common/exceptions/DrillRemoteException.java, > > line 22 > > <https://reviews.apache.org/r/32987/diff/2/?file=921683#file921683line22> > > > > purpose? If a DrillPBError object is sent passed from a fragment to the Foreman, we used to extract the error message from this object and then build a new error object, replacing the drillbit endpoint and creating a new error id along the way. This class will wrap a DrillPBError object a return instead of building a new error object, which will preserve the error information sent from the fragments. This class replaces RemoteRpcException because it was only used for this purpose > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > common/src/main/java/org/apache/drill/common/exceptions/DrillSystemException.java, > > line 23 > > <https://reviews.apache.org/r/32987/diff/2/?file=921684#file921684line23> > > > > This should be used only by the internal client layer, right? If so, > > let's make sure the constructors are as protected as possible. True, we should only create system exceptions through `DrillSystemException.wrapAndSetEndpoint(Throwable, DrillbitEndpoint)`. I will make DrillSystemException constructor _private_ > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java, > > line 71 > > <https://reviews.apache.org/r/32987/diff/2/?file=921686#file921686line71> > > > > Can we add method like getVerboseMessage() and then choose which one we > > want rather than controlling on creation? Will add it. Still, getMessage() is not the verbose message, it only contains the minimum amount of information we want to return to the client: - error type - error message - any context information available - error_id + drillbit_endpoint getVerboseMessage() will also add the stack trace of the exception that caused the error, this will be generated when building the DrillPBError object. > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > common/src/main/java/org/apache/drill/common/exceptions/DrillUserException.java, > > line 110 > > <https://reviews.apache.org/r/32987/diff/2/?file=921686#file921686line110> > > > > Do we have a separate error object type for bit <> bit? Do we send error objects between bit <> bit ? DrillPBError is only used between bit <> Foreman <> client. I am not aware of other error object types. > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java, > > line 181 > > <https://reviews.apache.org/r/32987/diff/2/?file=921689#file921689line181> > > > > Why do we need this? I will change it to a debug message > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java, > > line 193 > > <https://reviews.apache.org/r/32987/diff/2/?file=921690#file921690line193> > > > > Why do we need this? If it is a debug message, let's set it at that. it is a debug message > On April 9, 2015, 6:05 p.m., Jacques Nadeau wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java, > > line 172 > > <https://reviews.apache.org/r/32987/diff/2/?file=921692#file921692line172> > > > > 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. This can definitely be improved, it's just an example of throwing DrillParseException. Using only the message from the _ValidationException_ we get something like this: org.eigenbase.util.EigenbaseContextException: From line 1, column 15 to line 1, column 17: Table 'dfs.data.t.jso' not found using ex.getCause().getMessage() will give us a much cleaner error message: Table 'dfs.data.t.jso' not found In both cases I think we can safely ignore the "Failure validating SQL", using a DrillParseException will already display a _PARSE ERROR_ at the beginning of the message - abdelhakim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32987/#review79537 ----------------------------------------------------------- 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 > >
