> On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 1295 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439241#file1439241line1295> > > > > 2048 is a magic number - purpose?
2GB. It's the amount of RAM we're going to allocate to the AM. There's a "TODO: OYA:" here to choose good defaults and make it configurable. I'll make the comment more clear what this does. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 1299 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439241#file1439241line1299> > > > > JavaDoc says this call is blocking. So waitUntilAccepted is probably > > not necessary. Great! I'll remove the wait thing. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 1614 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439241#file1439241line1614> > > > > This method is too long It's actually shorter than the MR version of this method. And I think splitting this out might make it much harder to read. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java, > > line 1640 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439241#file1439241line1640> > > > > What if appStatus is null and fallback is false? Then we go to the else statement because the App is still RUNNING. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/util/ClasspathUtils.java, line 49 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439246#file1439246line49> > > > > I would pass this as a parameter to avoid storing state in the class. If we do that, then JavaActionExecutor has to know if it's in a minicluster or not. The way it is now, XTestCase sets this to true for tests, and no other code has to be aware of it. I don't like putting test-related code into production code, but this is needed here, and it's not too invasive. By the way, the equivalent code in Hadoop does this via a lookup in the Configuration, which is more expensive than the simple boolean I used. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > core/src/test/java/org/apache/oozie/QueryServlet.java, line 31 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439248#file1439248line31> > > > > I suppose this class is just temporary, right? No, it's used for some unit tests in TestCallbackNotifier. We have a number of misc tiny servlets doing various things that are used for tests. It probably would be a good idea to better organize them (they're currently all over the place), but that would be another JIRA. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java, > > line 380 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439250#file1439250line380> > > > > Why array? This is a trick to get around the fact that yarnClient needs to be final (because of the inner class), but I'm assigning it in the try block, and I need it to start out as null for the finally check to close it. It's an unfortunate trick that's sometimes needed for this sort of situation. Try removing the array stuff and see what happens :) > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/CallbackNotifier.java, > > line 55 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439264#file1439264line55> > > > > I would make these final if they don't change All but one of them changes. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java, > > line 121 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439265#file1439265line121> > > > > This method is extremely long, certain parts should be factored out to > > private methods. > > > > Especially because it has nested try-finally stuff inside. I did simplify (and improve a few things) over the MapperLauncher version, but I can do some more. > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java, > > line 286 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439265#file1439265line286> > > > > StringUtils.isEmpty ? Given that this is the AM, I don't want to add the entirety of Commons just for this method. But I'll change the equals empty string if statement to check the length instead (which is what isEmpty does). > On July 8, 2016, 1:28 p.m., Peter Bacsko wrote: > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java, > > line 342 > > <https://reviews.apache.org/r/49778/diff/1/?file=1439265#file1439265line342> > > > > Shorter: > > > > IOUtils.sizeOf() > > IOUtils.readFileToString() > > Peter Bacsko wrote: > Fix: not IOUtils, but FileUtils (commons-io) We'd have to add commons-io, which might be worth it for a more complicated function like this. However, we enforce a limit on the data size, which we can't do with FileUtils. I think our current code here is fine, it's been around since 2013 in LauncherMapper. Though I will replace StringBuffer with StringBuilder. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49778/#review141263 ----------------------------------------------------------- On July 8, 2016, 12:17 a.m., Robert Kanter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49778/ > ----------------------------------------------------------- > > (Updated July 8, 2016, 12:17 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 > >
