----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3021/#review3666 -----------------------------------------------------------
Needs some finishing touches to make the patch complete. trunk/core/src/main/java/org/apache/oozie/util/XLogStreamer.java <https://reviews.apache.org/r/3021/#comment8167> A more concise form to avoid repeated calls to getStackTrace can be: for(StackTraceElement stackTraceElem: ex.getStackTrace()) { sb.append(stackTraceElem + "\n"); } trunk/core/src/main/java/org/apache/oozie/util/XLogStreamer.java <https://reviews.apache.org/r/3021/#comment8171> Can you annotate this as an unexpected behavior and log the details instead of printing it to stderr? trunk/core/src/main/java/org/apache/oozie/util/XLogStreamer.java <https://reviews.apache.org/r/3021/#comment8168> Why is this message printed to stderr and not logged? trunk/core/src/main/java/org/apache/oozie/util/XLogStreamer.java <https://reviews.apache.org/r/3021/#comment8169> Same comment as above. Why is this message printed to stderr and not logged? trunk/core/src/main/java/org/apache/oozie/util/XLogStreamer.java <https://reviews.apache.org/r/3021/#comment8172> At this point, the value of returnVal is not known. Set it to -1 trunk/core/src/test/java/org/apache/oozie/util/TestLogStreamer.java <https://reviews.apache.org/r/3021/#comment8174> Can you add comments about what is being done in the block of code below? trunk/core/src/test/java/org/apache/oozie/util/TestLogStreamer.java <https://reviews.apache.org/r/3021/#comment8175> Since new assertions are being added, its a good time to document what is being tested and why trunk/core/src/test/java/org/apache/oozie/util/TestLogStreamer.java <https://reviews.apache.org/r/3021/#comment8176> Since new assertions are being added, its a good time to add what is being tested and why trunk/core/src/test/java/org/apache/oozie/util/TestLogStreamer.java <https://reviews.apache.org/r/3021/#comment8177> The test coverage has to be improved. The return value for -1 and 0 are being tested. The positive return values are not being tested. trunk/core/src/test/java/org/apache/oozie/util/TestLogStreamer.java <https://reviews.apache.org/r/3021/#comment8179> Very clever! Is this the recommended mechanism of testing private methods? Also add a comment here if this is the recommended mechanism. trunk/core/src/test/java/org/apache/oozie/util/TestLogStreamer.java <https://reviews.apache.org/r/3021/#comment8178> Can this be changed to a check for 0 instead of a check for non -1? - Santhosh On 2011-12-06 03:34:49, Kiran Nagasubramanian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3021/ > ----------------------------------------------------------- > > (Updated 2011-12-06 03:34:49) > > > Review request for oozie, Mohammad Islam and Angelo K. Huang. > > > Summary > ------- > > When retrieving the log content from compressed log files(gzip format), > unexpected exceptions are raised when file names dont conform to the standard > format.Those exceptions need to be handled gracefully. > > > This addresses bug OOZIE-627. > https://issues.apache.org/jira/browse/OOZIE-627 > > > Diffs > ----- > > trunk/core/src/main/java/org/apache/oozie/util/XLogStreamer.java 1210717 > trunk/core/src/test/java/org/apache/oozie/util/TestLogStreamer.java 1210717 > trunk/release-log.txt 1210717 > > Diff: https://reviews.apache.org/r/3021/diff > > > Testing > ------- > > Yes > > > Thanks, > > Kiran > >
