> On July 13, 2016, 10:51 p.m., Karthik Kambatla wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 1162 > > <https://reviews.apache.org/r/49778/diff/2/?file=1443093#file1443093line1162> > > > > Isn't RunningJob an MR thing?
Yes. We'll remove that eventually (there's a "TODO: OYA:" above the comments above it), but we need it for now in order to compile. I need to look into what this recovery stuff is and how to deal with it in Yarn. > On July 13, 2016, 10:51 p.m., Karthik Kambatla wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 1582 > > <https://reviews.apache.org/r/49778/diff/2/?file=1443093#file1443093line1582> > > > > Looks like createYarnClient is called from multiple places. Can't it be > > created once and cached? Hmm. That's an interesting thought. We actually do something like that for UserGroupInformations (UserGroupInformationService). It might make sense to keep a cache of YarnClients for each user around. Looking at UserGroupInformationService, it doesn't look like we ever remove anything from the cache except on shutdown though. Would there be any overhead (on Oozie or Yarn) for keeping the YarnClients around? Though this might get tricky if some code is using the YarnClient while/after it rolls off the cache and is closed. That might be the reason the UGIs aren't ever removed. Let's discuss this tomorrow in person. > On July 13, 2016, 10:51 p.m., Karthik Kambatla wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 1636 > > <https://reviews.apache.org/r/49778/diff/2/?file=1443093#file1443093line1636> > > > > Instead of the current approach of > > > > if (appStatus != null || fallback) { > > ... > > if (fallback) { > > ... > > appStatus = ... > > } > > } > > > > how about > > > > if (fallback) { > > ... > > appStatus = > > } > > > > if (appStatus != null) { > > ... > > } > > > > If avoiding creating actionData an extra time is bad, how about lazily > > creating it and reusing it the next time? But there's actually a lot of overlap here between the two cases. The inner ````if (fallback)```` does not have an else after it, so unless you get to the case where an exception is thrown, they do mostly the same thing. > On July 13, 2016, 10:51 p.m., Karthik Kambatla wrote: > > core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java, > > line 522 > > <https://reviews.apache.org/r/49778/diff/2/?file=1443097#file1443097line522> > > > > Helper method to create YarnClient in Oozie? Yes. It handles the impersonation and creating it for the correct Yarn cluster. > On July 13, 2016, 10:51 p.m., Karthik Kambatla wrote: > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/CallbackNotifier.java, > > line 44 > > <https://reviews.apache.org/r/49778/diff/2/?file=1443116#file1443116line44> > > > > Given there are two distinct callbacks here - one this and one the > > AMRMClientCallback, do you think there is a need to be explicit in naming > > this class? It's in an Oozie package, but I guess it could be confusing. I'll rename it. > On July 13, 2016, 10:51 p.m., Karthik Kambatla wrote: > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java, > > line 77 > > <https://reviews.apache.org/r/49778/diff/2/?file=1443117#file1443117line77> > > > > Does Oozie still support Java versions older than Java7? If not, one > > can drop the types when calling the constructor. Yes. We're going to change that soon, but for now, we're doing Java 6. > On July 13, 2016, 10:51 p.m., Karthik Kambatla wrote: > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java, > > line 141 > > <https://reviews.apache.org/r/49778/diff/2/?file=1443117#file1443117line141> > > > > +1. > > > > It would help to break this up further. Especially, the try-finally > > blocks are hard to read. e.g. verifying registerWithRM has an associated > > unregister was not obvious. Okay. Everyone's been complaining about this. I'll spend some more time playing around with this soon. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49778/#review142130 ----------------------------------------------------------- On July 13, 2016, 5:42 a.m., Robert Kanter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49778/ > ----------------------------------------------------------- > > (Updated July 13, 2016, 5:42 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2590 > https://issues.apache.org/jira/browse/OOZIE-2590 > > > Repository: oozie-git > > > Description > ------- > > See OOZIE-2590 > > > Diffs > ----- > > core/pom.xml 6584af8123925e00f7b29d6b33201f4fc7dca231 > core/src/main/java/org/apache/oozie/action/hadoop/DistcpActionExecutor.java > 96726daa800861c15e4a0d97514fbd45e31fa000 > core/src/main/java/org/apache/oozie/action/hadoop/Hive2ActionExecutor.java > b5b1bf90891fde1b13ca058e0b5f538b1b683128 > core/src/main/java/org/apache/oozie/action/hadoop/HiveActionExecutor.java > c74e9e61c01a4b6430bbd5491c8cbaf0c4e90198 > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 99e33445d68ac6a99a08e02ecc182f35103a53ce > core/src/main/java/org/apache/oozie/action/hadoop/LauncherMapperHelper.java > 69e1044e531a67c2d522f642376f4ad030423e53 > core/src/main/java/org/apache/oozie/action/hadoop/SparkActionExecutor.java > 252f461267818c0da91c7ad59380b2f18a074f9f > core/src/main/java/org/apache/oozie/action/hadoop/SqoopActionExecutor.java > 6813a37d0ffc66da41636fe01e9cc7a37d6f6e79 > core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java > 794e8258472df5fe18299eb3c62471cf9fecc186 > core/src/main/java/org/apache/oozie/util/ClasspathUtils.java PRE-CREATION > core/src/main/resources/oozie-default.xml > 6c2f7d82e933614ea6b434f9b035dca3751d1c7f > core/src/test/java/org/apache/oozie/QueryServlet.java PRE-CREATION > core/src/test/java/org/apache/oozie/action/hadoop/TestCallbackNotifier.java > PRE-CREATION > > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > 879bfebf58acc12ce787b151a67eaaa0716ff2bb > core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java > PRE-CREATION > > core/src/test/java/org/apache/oozie/action/hadoop/TestPrepareActionsDriver.java > df9e939cb236e8b65e7b3aa9ab683c33f5fa02b7 > > core/src/test/java/org/apache/oozie/action/hadoop/TestShellActionExecutor.java > 6a962a1030a16e61a2af27a6316c4d71b17ea5c9 > core/src/test/java/org/apache/oozie/command/wf/HangServlet.java > 3344cf97762c77f5660d94341e72df4243f76eb2 > core/src/test/java/org/apache/oozie/service/TestConfigurationService.java > 2153bf122ddbdf81acd90c4922fa8d0f619d2d41 > core/src/test/java/org/apache/oozie/service/TestHadoopAccessorService.java > 96faa4878111baafb81fdf0f3b38becba0c6f15e > core/src/test/java/org/apache/oozie/test/XTestCase.java > e3603692b2ae2ebb741adfa3a6ab5a37ea84eb96 > core/src/test/java/org/apache/oozie/util/TestClasspathUtils.java > PRE-CREATION > sharelib/distcp/pom.xml c8cc47cddc6d2922766e2d68ddddee96f5f3cfec > sharelib/hcatalog/pom.xml 2b0c5042a6f295101f8c5fae0b08a4239b5e97d3 > sharelib/hive/pom.xml d10d7b85b70cbeb1906c83484739f34c09038134 > sharelib/hive2/pom.xml ce967c5575eb639335332dc6de7ce3f3c66a55ca > sharelib/oozie/pom.xml dd95b4537c00b516b124341bdacf4fbd1fd351f9 > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/CallbackNotifier.java > PRE-CREATION > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java > PRE-CREATION > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMapper.java > f2cba13e89e8ddc16b59a04947309147ea6b34b7 > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java > 21ae456b768fb1cb5334a9e23e6c6082240f8dff > sharelib/pig/pom.xml 562c5309cde72c7430fc03f4d342b6d30b5b4575 > sharelib/spark/pom.xml 46c6375314ac314fc6e702695303bbc8eda4f87b > sharelib/sqoop/pom.xml d875c93ae599b990cdcbefdf2aa306eae41a9f0c > sharelib/streaming/pom.xml fd795182f11f0f884177a56592e99aaea01d195d > > Diff: https://reviews.apache.org/r/49778/diff/ > > > Testing > ------- > > - Tried a bunch of stuff against a Hadoop 2.4.0 cluster, including an > "action" that would have different kinds of Exceptions, System.exit, etc. > - Some new and old unit tests. > > > Thanks, > > Robert Kanter > >
