----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57107/#review168443 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/BaseEngine.java Lines 176-182 (patched) <https://reviews.apache.org/r/57107/#comment240698> Code duplication: can you just please use template method pattern? core/src/main/java/org/apache/oozie/BaseEngine.java Line 192 (original), 197-203 (patched) <https://reviews.apache.org/r/57107/#comment240699> Code duplication: can you just please use template method pattern? core/src/main/java/org/apache/oozie/BaseEngine.java Line 204 (original), 216-221 (patched) <https://reviews.apache.org/r/57107/#comment240700> Code duplication: can you just please use template method pattern? core/src/main/java/org/apache/oozie/util/AuthUrlClient.java Lines 44 (patched) <https://reviews.apache.org/r/57107/#comment240702> Would be better to name it with the unit of measure, e.g. `SERVER_SERVER_CONNECTION_TIMEOUT_SECONDS`. core/src/main/java/org/apache/oozie/util/AuthUrlClient.java Lines 132 (patched) <https://reviews.apache.org/r/57107/#comment240701> Should use a default timeout if configuration key is not present. core/src/main/java/org/apache/oozie/util/XLogAuditStreamer.java Lines 39 (patched) <https://reviews.apache.org/r/57107/#comment240703> Would be nice to have a sane default value. core/src/main/java/org/apache/oozie/util/XLogAuditStreamer.java Lines 68-77 (patched) <https://reviews.apache.org/r/57107/#comment240704> 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. core/src/main/java/org/apache/oozie/util/XLogFilter.java Lines 251 (patched) <https://reviews.apache.org/r/57107/#comment240705> Please rename to `calculateAndCheckDates()`. core/src/main/java/org/apache/oozie/util/XLogFilter.java Lines 302-305 (patched) <https://reviews.apache.org/r/57107/#comment240706> This check should come just after `startDate` and `endDate` have both been set. core/src/main/java/org/apache/oozie/util/XLogStreamer.java Lines 62-64 (patched) <https://reviews.apache.org/r/57107/#comment240707> 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. core/src/main/java/org/apache/oozie/util/XLogStreamer.java Lines 318-327 (patched) <https://reviews.apache.org/r/57107/#comment240708> 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. core/src/main/java/org/apache/oozie/util/XLogStreamer.java Lines 334-335 (patched) <https://reviews.apache.org/r/57107/#comment240709> Using `String.format()`, a `StringBuilder` instance, or Guava's `Objects.toStringHelper()` would be way sexier. core/src/main/resources/oozie-default.xml Lines 207 (patched) <https://reviews.apache.org/r/57107/#comment240710> `oozie.service.XLogStreamingService.audit.buffer.lineCount` would be a better name. core/src/main/resources/oozie-default.xml Lines 2235 (patched) <https://reviews.apache.org/r/57107/#comment240711> `oozie.server.connection.timeout.seconds` would be a better name. core/src/main/resources/oozie-default.xml Lines 2236 (patched) <https://reviews.apache.org/r/57107/#comment240712> I don't see how milliseconds could be useful here - setting to `180` would be better. core/src/test/java/org/apache/oozie/service/TestXLogStreamingService.java Lines 407 (patched) <https://reviews.apache.org/r/57107/#comment240713> Extracting to an `int fifteenHoursInSeconds` local variable would be more helpful. core/src/test/java/org/apache/oozie/util/TestLogStreamer.java Lines 347 (patched) <https://reviews.apache.org/r/57107/#comment240714> It would be useful to store here the original `XLogService.LOG4J_FILE` and then set it back in `tearDown()` for subsequent test runs. - András Piros On March 8, 2017, 6:56 p.m., Purshotam Shah wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57107/ > ----------------------------------------------------------- > > (Updated March 8, 2017, 6:56 p.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/1/ > > > Testing > ------- > > > Thanks, > > Purshotam Shah > >
