----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3143/#review4986 -----------------------------------------------------------
The error handling needs improvement as exceptions are not chained. trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3143/#comment10959> Exceptions should be chained to prevent loss of details. Any reason to not chain the exception here? trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment10975> Can you add a description of the class? trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment10968> Can we chain the exception? trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment10969> Can we chain the exception? trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment10970> Lack of details will hurt debugging - please include the appropriate error message from the previous line. trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment10971> Lack of details will hurt debugging - please include the appropriate error message from the previous line. trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment10972> Lack of details will hurt debugging - please include the appropriate error message from the previous line. trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3143/#comment10973> Can you chain the exception here? trunk/core/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java <https://reviews.apache.org/r/3143/#comment10976> Can you add a description of the class? trunk/core/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java <https://reviews.apache.org/r/3143/#comment10978> The error message may not be representative of the cause since Exception is being caught. If the error message is to be retained then try catching the appropriate exception thrown by getDocumentFromXML, i.e., ParserConfigurationException, SAXException, IOException trunk/core/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java <https://reviews.apache.org/r/3143/#comment10977> Can you chain the exception? trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestFileSystemActions.java <https://reviews.apache.org/r/3143/#comment10983> Similar to the tests for PrepareActionsDriver, delete the directory prior to the creation. trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestFileSystemActions.java <https://reviews.apache.org/r/3143/#comment10979> A better test would be to catch LauncherException and check for a reasonable error message and fail for Exception - something like: } catch (LauncherException le) { assertEquals(<check for message>); return; } catch (Exception ex) { <fail here> } <fail here too> if no exception was thrown trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestFileSystemActions.java <https://reviews.apache.org/r/3143/#comment10980> A better test would be to catch LauncherException and check for a reasonable error message and fail for Exception - something like: } catch (LauncherException le) { assertEquals(<check for message>); return; } catch (Exception ex) { <fail here> } <fail here too> if no exception was thrown trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestFileSystemActions.java <https://reviews.apache.org/r/3143/#comment10981> This code looks like a copy/paste of the method from PrepareActionsDriver - can this be refactored to avoid the copy? trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestPrepareActionsDriver.java <https://reviews.apache.org/r/3143/#comment10982> A better test would be to catch LauncherException and check for a reasonable error message and fail for Exception - something like: } catch (LauncherException le) { assertEquals(<check for message>); } catch (Exception ex) { <fail here> } - Santhosh On 2012-02-08 21:06:03, Kiran Nagasubramanian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3143/ > ----------------------------------------------------------- > > (Updated 2012-02-08 21:06:03) > > > Review request for oozie, Mohammad Islam and Angelo K. Huang. > > > Summary > ------- > > a) Separate classes for different types of actions > ------------------------------------------------ > The different types of actions like FS actions, HCat related actions, etc. > can be grouped together in separate classes like FSActions, HCatActions, etc. > > b) Passing the prepare logic to the Launcher > ----------------------------------------- > Launcher needs the Prepare XML block to execute the actions. Oozie server > can write the XML block to a file on DFS and then the Launcher could read > from there. > > c) Execution of actions through a "Driver" > --------------------------------------- > The Launcher can pass the XML block to the Driver which parses the content > and calls corresponding methods that are grouped in different classes. > > > This addresses bug OOZIE-616. > https://issues.apache.org/jira/browse/OOZIE-616 > > > Diffs > ----- > > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java > PRE-CREATION > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 1213901 > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java > 1213901 > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestFileSystemActions.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > 1213901 > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncher.java > 1213901 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionError.java > 1213901 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java > 1213901 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestPigActionExecutor.java > 1213901 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestPrepareActionsDriver.java > PRE-CREATION > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestShellActionExecutor.java > 1213901 > > Diff: https://reviews.apache.org/r/3143/diff > > > Testing > ------- > > Yes > > > Thanks, > > Kiran > >
