----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13397/#review25280 -----------------------------------------------------------
trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/13397/#comment49566> Why remove private? trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/13397/#comment49576> This can be a local variable inside check() method. Don't need a class variable. trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/13397/#comment49569> Avoid passing objects and modifying them in methods. Make the method return a map as the return type. trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/13397/#comment49574> Don't have to make hasIdSwap call and check the counter. You can just check if the map has the key. Please create another jira to remove code related to setting and checking of the counter or if possible do it in this jira itself. trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/13397/#comment49567> Can store the id directly as String instead of having Properties. trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/13397/#comment49571> If there was an id swap, i think you need to reload the action data here. trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java <https://reviews.apache.org/r/13397/#comment49570> Remove the hasOutputData and hasStatsData checks. containsKey should be good enough trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapperHelper.java <https://reviews.apache.org/r/13397/#comment49575> getActionData(..) Please make the method return the map. Do not take it as argument and put values in it. Not a good practice. trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapperHelper.java <https://reviews.apache.org/r/13397/#comment49577> Do a file exists check and return empty map if the file does not exist. The file will not exist if the launcher did not run at all. There should be no fs.getHomeDirectory(). The path should just be getActionDataSequenceFilePath(actionDir) trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapperHelper.java <https://reviews.apache.org/r/13397/#comment49578> This is not required. Just do new Text(). trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapperHelper.java <https://reviews.apache.org/r/13397/#comment49579> Null check not required. If any of it was null reading the file would fail trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncher.java <https://reviews.apache.org/r/13397/#comment49581> Can be removed. trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncher.java <https://reviews.apache.org/r/13397/#comment49582> Why remove this? trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncher.java <https://reviews.apache.org/r/13397/#comment49583> assertFalse(fs.exists(LauncherMapperHelper.getActionDataSequenceFilePath(actionDir))); Same in other places trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncher.java <https://reviews.apache.org/r/13397/#comment49580> To be cleaned? trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/13397/#comment49572> The file name will become oozie.action.data.seq. Just have action-data.seq or action_data.seq. No . in the middle of the file name. trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/13397/#comment49585> This should be in a finally block after LauncherException catch block. You are only creating the file in case of an exception now. trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/13397/#comment49584> Path finalPath = new Path(actionDir, ACTION_DATA_SEQUENCE_FILE); trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/13397/#comment49586> You don't need hadoop filesystem to read a File. Just read using BufferedInputStream and FileInputStream. Also do not read line by line. Read as is into byte array. Currently you are reading line by line and appending to stringbuffer without appending \n. Can do FileUtils.readFileToString() from commons-io package, but commons-io is not present in hadoop 1.x. Only in 0.23. trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java <https://reviews.apache.org/r/13397/#comment49573> It should be prepareXML. actionXml is the file that contains the action configuration. - Rohini Palaniswamy On Aug. 16, 2013, 9:31 a.m., Mona Chitnis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13397/ > ----------------------------------------------------------- > > (Updated Aug. 16, 2013, 9:31 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1486 > https://issues.apache.org/jira/browse/OOZIE-1486 > > > Repository: oozie > > > Description > ------- > > See JIRA Description > > > Diffs > ----- > > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/HiveActionExecutor.java > 1514596 > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 1514596 > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapperHelper.java > 1514596 > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/PigActionExecutor.java > 1514596 > > trunk/core/src/main/java/org/apache/oozie/action/hadoop/SqoopActionExecutor.java > 1514596 > trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestLauncher.java > 1514596 > > trunk/sharelib/hive/src/main/java/org/apache/oozie/action/hadoop/HiveMain.java > 1514596 > > trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java > 1514596 > > Diff: https://reviews.apache.org/r/13397/diff/ > > > Testing > ------- > > Existing unit tests should pass, and E-2-E test pending. > > Few tests failing with a common problem of Properties object (for > output/new-id/error etc) getting stored in the file with Timestamp. Hence on > retrieval, it gives NPE on the expected keys. WIP to fix that small part > > > Thanks, > > Mona Chitnis > >
