----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3143/#review3848 -----------------------------------------------------------
trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8667> this should be done in a static {} so it done once and no risk or concurrency issues in the HashSet. Or the variables should be instance variables. and the isSupportedFileSsytemSetConstructor is not needed. trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8668> why equalIgnoreCase(), equal() should be used, per schema def. trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8669> ditto as prev comment trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8670> why this?, this is already done in the constructor trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8671> remove trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8672> this eats the LauncherException thrown within the try/catch, it should not. use a catch for it before and rethrow it from there trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8673> the cause exception should be passed as param trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8674> same comments as in delete trunk/core/src/main/java/org/apache/oozie/action/hadoop/FileSystemActions.java <https://reviews.apache.org/r/3143/#comment8675> what is this? seems overly complex, we should have a static fixed set of supported FS and we are good, no? trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3143/#comment8676> as this is to be used by the FS action as well, the more correct name & value would be FILE_SYSTEM_OPS_XML trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3143/#comment8677> why writing to a separate file instead setting it as property in the action.xml ? (we have to start thinking about less file in HDFS) trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3143/#comment8678> remove, this is on oozie server trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3143/#comment8679> executeFileSystemOperations is more appropriate as method name trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/3143/#comment8680> again, if using a property to encode all the fs operations, this is much simpler trunk/core/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java <https://reviews.apache.org/r/3143/#comment8681> Why not using JDOM and/or Oozie XML utils? then this class is not needed - Alejandro On 2011-12-12 18:23:46, Kiran Nagasubramanian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3143/ > ----------------------------------------------------------- > > (Updated 2011-12-12 18:23:46) > > > 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 > 1211045 > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java > 1211045 > > 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 > 1211045 > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncher.java > 1211045 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionError.java > 1211045 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestMapReduceActionExecutor.java > 1211045 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestPigActionExecutor.java > 1211045 > > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestPrepareActionsDriver.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/3143/diff > > > Testing > ------- > > Yes > > > Thanks, > > Kiran > >
