> On júl. 8, 2016, 1:28 du, 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? > > Robert Kanter wrote: > 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.
Just said because I wasn't sure about the purpose of this servlet (seemed like a temporary hack). I'd add a comment here to just to clarify this. > On júl. 8, 2016, 1:28 du, 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? > > Robert Kanter wrote: > 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 :) I see, in this case it's indeed necessary. However it still looks confusing. IMO it's worth an extra comment on this and perhaps using MutableObject from commons-lang would look nicer (just an idea). > On júl. 8, 2016, 1:28 du, 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 ? > > Robert Kanter wrote: > 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). Hm, didn't see that it's sharelib, OK. > On júl. 8, 2016, 1:28 du, 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) > > Robert Kanter wrote: > 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. Mmm, sharelib again :) Then forget about it. > On júl. 8, 2016, 1:28 du, 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 > > Robert Kanter wrote: > It's actually shorter than the MR version of this method. And I think > splitting this out might make it much harder to read. Maybe I was just fooled by the amount of diff. Let's drop this for now and maybe address it later. > On júl. 8, 2016, 1:28 du, 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. > > Robert Kanter wrote: > 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. OK, sounds reasonable. - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49778/#review141263 ----------------------------------------------------------- On júl. 13, 2016, 5:42 de, Robert Kanter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49778/ > ----------------------------------------------------------- > > (Updated júl. 13, 2016, 5:42 de) > > > 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 > >
