> On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/BaseEngine.java > > Line 192 (original), 197-203 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659376#file1659376line202> > > > > Code duplication: can you just please use template method pattern?
It's not duplicate code. Each function takes different streamer. > On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/BaseEngine.java > > Line 204 (original), 216-221 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659376#file1659376line221> > > > > Code duplication: can you just please use template method pattern? It's not duplicate code. Each function takes different streamer. > On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/util/XLogFilter.java > > Lines 302-305 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659390#file1659390line304> > > > > This check should come just after `startDate` and `endDate` have both > > been set. Not sure what you mean by this? > On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/util/XLogStreamer.java > > Lines 62-64 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659391#file1659391line65> > > > > Since the parameter `logFile` shadows the field `logFile`, and the > > field `logFile` has already been set, no effect is taken by that, breaking > > the functionality. > > > > Either move that check-and-set back to its original place or remove it > > completely. You are right. Intention was to set this.logFile = "oozie-app.log"; not logFile = "oozie-app.log". This is one of the reasons we should avoid the same name for global and local variables. > On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/util/XLogStreamer.java > > Lines 318-327 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659391#file1659391line321> > > > > Parameter `writtenBytes` is not used. Is it intentional? If so, please > > rename it, like `byteCountIgnored`. > > > > Moreover, please rename the method to e.g. `checkAndFlush()` to better > > reflect what it actually does. writtenBytes is used. checkAndFlush may not be an appropriate name because the function doesn't call flush. It only says should caller flush or not. > On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/util/XLogStreamer.java > > Lines 334-335 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659391#file1659391line337> > > > > Using `String.format()`, a `StringBuilder` instance, or Guava's > > `Objects.toStringHelper()` would be way sexier. Agree. > On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/resources/oozie-default.xml > > Lines 207 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659392#file1659392line207> > > > > `oozie.service.XLogStreamingService.audit.buffer.lineCount` would be a > > better name. This is debatbale. In most of the we just replace the type to change to value. > On March 9, 2017, 1:36 p.m., András Piros wrote: > > core/src/main/resources/oozie-default.xml > > Lines 2235 (patched) > > <https://reviews.apache.org/r/57107/diff/1/?file=1659392#file1659392line2235> > > > > `oozie.server.connection.timeout.seconds` would be a better name. hmmm... Most of are value connection timeouts are set in millisecond like oozie.command.default.lock.timeout, oozie.notification.url.connection.timeout. Setting in seconds is more favorable because we are never going to set it in ms. - Purshotam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57107/#review168443 ----------------------------------------------------------- On March 14, 2017, 2:03 a.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57107/ > ----------------------------------------------------------- > > (Updated March 14, 2017, 2:03 a.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2815 > https://issues.apache.org/jira/browse/OOZIE-2815 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-2815 oozie not always display job log > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/BaseEngine.java > 50df8978773ed54fb5a39bb142d7235de1ed396f > core/src/main/java/org/apache/oozie/BundleEngine.java > d0099b48772069217375c032d171e8dc6b1bbbbd > core/src/main/java/org/apache/oozie/CoordinatorEngine.java > 2f9f8227f1b2bbb0074cfa19aac37ab9a3fe7c0f > core/src/main/java/org/apache/oozie/DagEngine.java > 57d276199de149b294f64570a8878b660e9a5a0c > core/src/main/java/org/apache/oozie/service/XLogService.java > 04f04f41403fc6446967524124b8cf86f816d71c > core/src/main/java/org/apache/oozie/service/XLogStreamingService.java > c15c4c1731838655d8f899f9c0405b226f8111b0 > core/src/main/java/org/apache/oozie/service/ZKXLogStreamingService.java > 97771ad22be84d1a8167c69a6b48b7012652cf8b > core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java > d3b468965f33e300d7ca0f31efc596fd969b47a5 > core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java > 9356768639e6e3ea548b25baa75d576351fa80e3 > core/src/main/java/org/apache/oozie/util/AuthUrlClient.java > b45a96acc397c26cdba8d5c62503013dd572f247 > core/src/main/java/org/apache/oozie/util/TimestampedMessageParser.java > a676f4d35a49147aadde73357397b211ab829c85 > core/src/main/java/org/apache/oozie/util/XLogAuditFilter.java > c377db5df5dd0bbeed77b4d5c88e1c4e2e64a51e > core/src/main/java/org/apache/oozie/util/XLogAuditStreamer.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/util/XLogErrorStreamer.java > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > core/src/main/java/org/apache/oozie/util/XLogFilter.java > 3b49f77e016cbeaedfa03708dbc680e68f40ca16 > core/src/main/java/org/apache/oozie/util/XLogStreamer.java > 19f1fee55ba8d6c456e798f01e55cfe84cfb5803 > core/src/main/resources/oozie-default.xml > 95e0c36e1b2f5aef18637f6f956a990ba36166f2 > core/src/test/java/org/apache/oozie/TestCoordinatorEngineStreamLog.java > 3eb1016f3ffccaa9db077580b90cc1b3c53fd479 > core/src/test/java/org/apache/oozie/service/TestConfigurationService.java > 42ffdbeb41e5829cdf814cf13ada331973fc33bd > core/src/test/java/org/apache/oozie/service/TestXLogStreamingService.java > bebb678798ec4a01c9710660d4b99fddd5167560 > core/src/test/java/org/apache/oozie/service/TestZKXLogStreamingService.java > fca8d844f1563da5856eff36f53f21d519630d31 > core/src/test/java/org/apache/oozie/util/TestLogStreamer.java > f90f784503e101ae2f96fee52977ee83f3098301 > > core/src/test/java/org/apache/oozie/util/TestSimplifiedTimestampedMessageParser.java > ea899faf00aa2f98f43838987c9b716321a50dea > core/src/test/java/org/apache/oozie/util/TestTimestampedMessageParser.java > 9e28cbc77129bb2675b92ec9c18c77accfd78a55 > core/src/test/java/org/apache/oozie/util/TestXLogUserFilterParam.java > 46f273fde7f14b446463448c90f22519761997b6 > webapp/src/main/webapp/oozie-console.js > 76864a9a06e98357c2d17e35609e2a76afbd595c > > > Diff: https://reviews.apache.org/r/57107/diff/2/ > > > Testing > ------- > > > Thanks, > > Purshotam Shah > >
