drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdelhakim deneche" <adene...@gmail.com>
Subject Re: Review Request 32987: DRILL-2675: Implement a subset of User Exceptions to improve how errors are reported to the user
Date Thu, 09 Apr 2015 17:20:07 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32987/
-----------------------------------------------------------

(Updated April 9, 2015, 5:20 p.m.)


Review request for drill and Jacques Nadeau.


Changes
-------

This is an improved patch. Error propagation has been improved, and it should be easier for
developer to send meaningful error messages to the client without having the message being
lost or mangled before reaching the client.

**Changes since the last patch**
- DrillSystemException will display the root error message of the wrapped exception
 . from the user perspective, this will keep Drill behaviour the same when reporting errors
that don't have **yet** a proper user error message
- DrillUserException.getMessage() generates the error message that will be sent to the client
 . we don't need ErrorHelper to build the message from protobuf error object
 . logging a DrillUserException will display the proper error message in the logs
- added DrillUnsupportedOperationException
- moved some error message generation logic from DrillParseException to DrillSqlWorker
- DrillUserException constructor expects an ErrorType. This allows it to build the final error
message itself
- DrillUserException.addContext(String) can be used to add more context to existing user exceptions
 . subclasses of DrillUserException don't need to hold specific context fields
 . it is easier to add context to an existing user exception without knowing it's specific
type
 . the error message generation can be centralized in DrillUserException.getMessage()
- the final error message is generated on the server side and sent to the client through a
DrillPBError's message field
 . we don't need to update the clients each time we add a new context field to user exceptions
- JsonRecordReader first checks if the exception catched is not already a user exception,
in this case it will just add more context to it and throw it, otherwise it will create a
DrillDataReadException
- reverted my changes in proto objects DrillPBError and QueryResult, the only change left
is the ErrorType enum

I still need to do some cleanup and add the remaining user exception types.


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 (updated)
-----

  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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message