----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52737/#review152128 -----------------------------------------------------------
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java (line 27) <https://reviews.apache.org/r/52737/#comment220904> This class called failLauncer() before. It's unsafe because these calls are invoked from a separate thread. IMO It's better to check errors synchronously at the end of LauncherAM.run(). sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 81) <https://reviews.apache.org/r/52737/#comment220902> TODO: add JavaDoc sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 102) <https://reviews.apache.org/r/52737/#comment220897> We can move this to the SeqFileWriter factory. A better name would be createReader not getReader. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 106) <https://reviews.apache.org/r/52737/#comment220903> TODO: add JavaDoc sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 235) <https://reviews.apache.org/r/52737/#comment220901> The callback notifier creates URL connections, which we don't want here. Since it also requires the launcher job conf, it cannot be instantiated in main() -- so we create it with a factory. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 243) <https://reviews.apache.org/r/52737/#comment220898> This map will be inspected in unit tests sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 292) <https://reviews.apache.org/r/52737/#comment220899> The reason for the factory is that the value "60000" will come from the launcher job xml. It's easier from failure handling POV if the xml is loaded in run(), not in main(). sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 482) <https://reviews.apache.org/r/52737/#comment220900> TODO: actionData will be removed - Peter Bacsko On okt. 11, 2016, 1:42 du, Peter Bacsko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52737/ > ----------------------------------------------------------- > > (Updated okt. 11, 2016, 1:42 du) > > > Review request for oozie, András Piros, Attila Sasvari, Peter Cseh, and > Robert Kanter. > > > Repository: oozie-git > > > Description > ------- > > This review contains a suggested approach for refactoring LauncherAM on the > OYA branch. > > The main idea is to inject all dependencies of the class via the constructor. > In some cases, this requires factory objects. Note that this is necessary > because in unit tests, because we don't want to create actual resources such > as UGI or async ResourceManager callback. > > In the tests, we can instantiate this class with mocks. The testability is > greatly enhanced because various checks and verifications can be performed on > the mocks. Tests are currently not in this review (see > https://issues.apache.org/jira/browse/OOZIE-2685). > > > Diffs > ----- > > core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java > ed29299 > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java > PRE-CREATION > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java > PRE-CREATION > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java > PRE-CREATION > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java > PRE-CREATION > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java > 85d78c6 > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java > 23648b8 > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java > PRE-CREATION > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java > 4a51d48 > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java > PRE-CREATION > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/52737/diff/ > > > Testing > ------- > > Unit tests are being written right now. > > > Thanks, > > Peter Bacsko > >
