-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/320/#review119
-----------------------------------------------------------
1) I like the simplification of not requiring the - in FS commands
2) In "catch (PyException e)" whether the message is null or not I would use
e.toString() which includes the Exception class name + message. A good example
of null message is NullPointerException. Usually the message is null when the
exception class name is self explanatory. A non null message may still require
the exception class name along with it to make sense. I don't think we need to
put the full stack trace in the message as the exception is passed as the cause
of the ExecException.
It would look like this:
} catch (PyException e) {
String message = "Python Error. "+e.toString();
throw new ExecException(message, 1121, e);
}
Regarding the code 1121, I would make it a generic scripting language error
code reused for other scripting error (like js).
- Julien
On 2011-01-14 09:51:20, Richard Ding wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/320/
> -----------------------------------------------------------
>
> (Updated 2011-01-14 09:51:20)
>
>
> Review request for pig and Julien Le Dem.
>
>
> Summary
> -------
>
> Current Pig exception handling does not treat Jython exceptions differently
> from general RuntimeExceptions. We need to put Jython exceptions in a
> separate class and output better error messages.
>
>
> This addresses bug PIG-1801.
> https://issues.apache.org/jira/browse/PIG-1801
>
>
> Diffs
> -----
>
>
> http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/scripting/Pig.java
> 1058309
>
> http://svn.apache.org/repos/asf/pig/trunk/src/org/apache/pig/scripting/jython/JythonScriptEngine.java
> 1058309
>
> http://svn.apache.org/repos/asf/pig/trunk/test/org/apache/pig/test/TestScriptLanguage.java
> 1058309
>
> Diff: https://reviews.apache.org/r/320/diff
>
>
> Testing
> -------
>
> [exec] +1 overall.
> [exec]
> [exec] +1 @author. The patch does not contain any @author tags.
> [exec]
> [exec] +1 tests included. The patch appears to include 3 new or
> modified tests.
> [exec]
> [exec] +1 javadoc. The javadoc tool did not generate any warning
> messages.
> [exec]
> [exec] +1 javac. The applied patch does not increase the total
> number of javac compiler warnings.
> [exec]
> [exec] +1 findbugs. The patch does not introduce any new Findbugs
> warnings.
> [exec]
> [exec] +1 release audit. The applied patch does not increase the
> total number of release audit warnings.
>
>
> Thanks,
>
> Richard
>
>
