----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59134/#review174437 -----------------------------------------------------------
core/pom.xml Lines 473 (patched) <https://reviews.apache.org/r/59134/#comment247581> Better to put `<version />` to root `pom.xml` inside `<dependencyManagement />`. core/pom.xml Lines 476 (patched) <https://reviews.apache.org/r/59134/#comment247570> One newline more than needed. core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java Lines 43-44 (patched) <https://reviews.apache.org/r/59134/#comment247571> Could and should be `final`. core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java Lines 47-50 (patched) <https://reviews.apache.org/r/59134/#comment247585> Just thinking about to put `initialWaitTimeMs` and `maxRetryCount` logic to `RetryerFactory` as `private static final` variables, and have a default parameterless `create()` to use those. No duplicated code would result. core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java Lines 57 (patched) <https://reviews.apache.org/r/59134/#comment247588> `Retryer` is [thread safe](https://github.com/rholder/guava-retrying/issues/35), so `retryer` could be `private static final`. Also, `RetryerFactory.create()` could be called for more clarity, and less code duplication. core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java Lines 49-51 (original), 61-71 (patched) <https://reviews.apache.org/r/59134/#comment247590> When extracted to a nested class, could be unit tested separately. core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java Line 49 (original), 65 (patched) <https://reviews.apache.org/r/59134/#comment247578> Please remove tabs. core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java Lines 50-51 (original), 67-68 (patched) <https://reviews.apache.org/r/59134/#comment247579> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 251-252 (patched) <https://reviews.apache.org/r/59134/#comment247586> Just thinking about to put `initialWaitTimeMs` and `maxRetryCount` logic to `RetryerFactory` as `private static final` variables, and have a default parameterless `create()` to use those. No duplicated code would result. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 283 (patched) <https://reviews.apache.org/r/59134/#comment247600> `Retryer` is [thread safe](https://github.com/rholder/guava-retrying/issues/35), so `retryer` could be `private static final`. Also, `RetryerFactory.create()` could be called for more clarity, and less code duplication. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 270-281 (original), 285-302 (patched) <https://reviews.apache.org/r/59134/#comment247594> When extracted to a nested class, could be tested separately. core/src/main/java/org/apache/oozie/service/JPAService.java Line 270 (original), 289 (patched) <https://reviews.apache.org/r/59134/#comment247591> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 271-280 (original), 292-301 (patched) <https://reviews.apache.org/r/59134/#comment247592> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 359 (patched) <https://reviews.apache.org/r/59134/#comment247599> `Retryer` is [thread safe](https://github.com/rholder/guava-retrying/issues/35), so `retryer` could be `private static final`. Also, `RetryerFactory.create()` could be called for more clarity, and less code duplication. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 331-341 (original), 361-376 (patched) <https://reviews.apache.org/r/59134/#comment247598> When extracted to a nested class, could be unit tested separately. core/src/main/java/org/apache/oozie/service/JPAService.java Line 331 (original), 365 (patched) <https://reviews.apache.org/r/59134/#comment247596> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 332-340 (original), 367-375 (patched) <https://reviews.apache.org/r/59134/#comment247597> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 453 (patched) <https://reviews.apache.org/r/59134/#comment247601> `Retryer` is [thread safe](https://github.com/rholder/guava-retrying/issues/35), so `retryer` could be `private static final`. Also, `RetryerFactory.create()` could be called for more clarity, and less code duplication. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 412-438 (original), 455-496 (patched) <https://reviews.apache.org/r/59134/#comment247605> If extracted to a nested class, could be unit tested separately. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 460-461 (patched) <https://reviews.apache.org/r/59134/#comment247602> Would extract to `private tryRollbackSafely()` - in this case no inline comments would be needed. core/src/main/java/org/apache/oozie/service/JPAService.java Line 412 (original), 467 (patched) <https://reviews.apache.org/r/59134/#comment247603> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 413-437 (original), 470-495 (patched) <https://reviews.apache.org/r/59134/#comment247604> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 523 (patched) <https://reviews.apache.org/r/59134/#comment247606> `Retryer` is [thread safe](https://github.com/rholder/guava-retrying/issues/35), so `retryer` could be `private static final`. Also, `RetryerFactory.create()` could be called for more clarity, and less code duplication. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 463-471 (original), 525-538 (patched) <https://reviews.apache.org/r/59134/#comment247611> When extracted to a nested class, could be unit tested separately. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 463-467 (original), 528-532 (patched) <https://reviews.apache.org/r/59134/#comment247607> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Line 467 (original), 532 (patched) <https://reviews.apache.org/r/59134/#comment247609> `catch (final NoResultException ignored)`? core/src/main/java/org/apache/oozie/service/JPAService.java Lines 533 (patched) <https://reviews.apache.org/r/59134/#comment247610> Maybe `DEBUG` level is better here. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 468-471 (original), 534-537 (patched) <https://reviews.apache.org/r/59134/#comment247608> Please remove tabs. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 569 (patched) <https://reviews.apache.org/r/59134/#comment247612> `Retryer` is [thread safe](https://github.com/rholder/guava-retrying/issues/35), so `retryer` could be `private static final`. Also, `RetryerFactory.create()` could be called for more clarity, and less code duplication. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 494-502 (original), 571-583 (patched) <https://reviews.apache.org/r/59134/#comment247614> When extracted to a nested class, could be unit tested separately. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 494-502 (original), 574-582 (patched) <https://reviews.apache.org/r/59134/#comment247613> Please remove tabs. core/src/main/java/org/apache/oozie/util/db/RetryHandlerFactory.java Lines 32 (patched) <https://reviews.apache.org/r/59134/#comment247577> Should be named `RetrierFactory`, then. core/src/main/java/org/apache/oozie/util/db/RetryHandlerFactory.java Lines 33 (patched) <https://reviews.apache.org/r/59134/#comment247575> Would be better to use `XLog.getLog(OozieRetryListener.class)`. core/src/main/java/org/apache/oozie/util/db/RetryHandlerFactory.java Lines 34 (patched) <https://reviews.apache.org/r/59134/#comment247587> Just thinking about to put `initialWaitTimeMs` and `maxRetryCount` logic to `RetryerFactory` as `private static final` variables, and have a default parameterless `create()` to use those. No duplicated code would result. core/src/main/java/org/apache/oozie/util/db/RetryHandlerFactory.java Lines 35-42 (patched) <https://reviews.apache.org/r/59134/#comment247574> Would be better to extract to a `private static final class OozieRetryListener` and instantiate it here as `private static final OozieRetryListener`. core/src/main/java/org/apache/oozie/util/db/RetryHandlerFactory.java Lines 39 (patched) <https://reviews.apache.org/r/59134/#comment247576> I'd have a `TRACE` level log also for any case, not just when an `Exception` happens. And I'd also log how many attempts yet to retry, and how many seconds till next attempt, as follows: ``` Attempt #4 of 10 failed, retrying in 15 seconds. ``` core/src/main/java/org/apache/oozie/util/db/RetryHandlerFactory.java Lines 44 (patched) <https://reviews.apache.org/r/59134/#comment247580> Better call it `create()`. core/src/main/resources/oozie-default.xml Lines 1567 (patched) <https://reviews.apache.org/r/59134/#comment247572> Would be better to name it `oozie.database.retry.initial-wait-time.ms` in order to express Unit of Measure. core/src/main/resources/oozie-default.xml Lines 1576 (patched) <https://reviews.apache.org/r/59134/#comment247573> Would be better to name it `oozie.database.retry.max-retries` to conform to naming convention across `oozie-default.xml`. - András Piros On May 10, 2017, 9:23 a.m., Peter Bacsko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59134/ > ----------------------------------------------------------- > > (Updated May 10, 2017, 9:23 a.m.) > > > Review request for oozie, András Piros and Robert Kanter. > > > Repository: oozie-git > > > Description > ------- > > https://issues.apache.org/jira/browse/OOZIE-2854 > > > Diffs > ----- > > core/pom.xml 14aa034 > core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java 8d94c23 > core/src/main/java/org/apache/oozie/service/JPAService.java 028381d > core/src/main/java/org/apache/oozie/util/db/RetryHandlerFactory.java > PRE-CREATION > core/src/main/resources/oozie-default.xml e7a48a0 > > > Diff: https://reviews.apache.org/r/59134/diff/1/ > > > Testing > ------- > > Tested on a 3-node cluster. MySQL was killed/restared. The workflow execution > was not disrupted. > > > Thanks, > > Peter Bacsko > >
