> On 2011-01-14 10:22:23, Julien Le Dem wrote:
> > 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).
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
Thanks Julien. The PyException is implemented as the full stack trace:
public synchronized String toString() {
ByteArrayOutputStream buf = new ByteArrayOutputStream();
if (!printingStackTrace) {
printStackTrace(new PrintStream(buf));
}
return buf.toString();
}
I'll change the code to use e.toString().
- Richard
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/320/#review119
-----------------------------------------------------------
On 2011-01-14 14:15:22, Richard Ding wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/320/
> -----------------------------------------------------------
>
> (Updated 2011-01-14 14:15:22)
>
>
> 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/ScriptEngine.java
> 1059179
>
> 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
>
>
