----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60544/#review179556 -----------------------------------------------------------
Please document the feature and its configuration in the twiki files. core/pom.xml Lines 39 (patched) <https://reviews.apache.org/r/60544/#comment254321> Do not add this dependency twice: https://github.com/apache/oozie/blob/378f294c2cd92ac0d505201857f03746ce2e58ac/core/pom.xml#L743 core/src/main/java/org/apache/oozie/service/JPAService.java Line 273 (original), 330 (patched) <https://reviews.apache.org/r/60544/#comment254347> - Extract method ``` if (em.getTransaction().isActive()) { ... em.getTransaction().commit(); } ``` - Extract constant "org.apache.oozie.command.SkipCommitFaultInjection" core/src/main/java/org/apache/oozie/service/JPAService.java Lines 351 (patched) <https://reviews.apache.org/r/60544/#comment254322> remove dead comnent core/src/main/java/org/apache/oozie/service/JPAService.java Line 333 (original), 399 (patched) <https://reviews.apache.org/r/60544/#comment254375> use ``commitTransaction(em)`` core/src/main/java/org/apache/oozie/service/JPAService.java Lines 486 (patched) <https://reviews.apache.org/r/60544/#comment254365> this comment is not necessary core/src/main/java/org/apache/oozie/service/JPAService.java Lines 490 (patched) <https://reviews.apache.org/r/60544/#comment254366> I am not sure if ``em.getTransaction().isActive()`` will ever be evaluated true. However, if so and rollback is not successfull, we should return and increase actual retry count (rollback is a kind of retry attempt). core/src/main/java/org/apache/oozie/service/JPAService.java Line 413 (original), 496 (patched) <https://reviews.apache.org/r/60544/#comment254364> Use ``&& !updateQueryList.isEmpty()`` instead of size() >0 core/src/main/java/org/apache/oozie/service/JPAService.java Line 499 (original), 599 (patched) <https://reviews.apache.org/r/60544/#comment254346> Log that no results were returned like in ``executeGet`` to keep consistency core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java Lines 71 (patched) <https://reviews.apache.org/r/60544/#comment254369> This a new named query that it is only used in ``XTestCase``, and there is no check is performed on it in any tests; it is is used in ``cleanUpDBTablesInternal()``. In other words, no REST API requests are using it for any tasks. As I see there are other named queries that are not used for meaningful purposes (i.e. they are only used in tests). It would be nice to go through all named queries and eliminate the "dead" ones. For example "GET_ACTIONS" is only used in tests. core/src/main/java/org/apache/oozie/util/db/BasicDataSourceWrapper.java Lines 33 (patched) <https://reviews.apache.org/r/60544/#comment254352> Could you add DBCP-333 to reveal what the known bug is? core/src/main/java/org/apache/oozie/util/db/BasicDataSourceWrapper.java Lines 41 (patched) <https://reviews.apache.org/r/60544/#comment254350> If driverClassName is null, we can return earlier throwing a SQLNestedException with appropriate message. This way it will not be needed to check a second time driverClassName is null. core/src/main/java/org/apache/oozie/util/db/BasicDataSourceWrapper.java Lines 42 (patched) <https://reviews.apache.org/r/60544/#comment254349> 1 try with multiple catch block should be enough core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java Lines 347 (patched) <https://reviews.apache.org/r/60544/#comment254360> nit: OozieDBCLI also uses the tablenames. We could extract these into a central place perhaps in a separate Jira? core/src/main/java/org/apache/oozie/util/db/OperationRetryHandler.java Lines 31 (patched) <https://reviews.apache.org/r/60544/#comment254329> No need for this annotation; if RETRY_ATTEMPT_COUNTER is package private, then TestOperationRetryHandler will access it. Or you can restrict visibility of RETRY_ATTEMPT_COUNTER to private. core/src/main/java/org/apache/oozie/util/db/OperationRetryHandler.java Lines 74 (patched) <https://reviews.apache.org/r/60544/#comment254362> As a user, this error message would confuse me. ``Retry attempts have been exhausted.`` / ``Database operation retry limit has been exceeded.`` would be more descriptive in my opinion. core/src/main/java/org/apache/oozie/util/db/OperationRetryHandler.java Lines 86 (patched) <https://reviews.apache.org/r/60544/#comment254363> nit: "Exception is not on blacklist" or something would be better than "Exception does not apply" core/src/main/java/org/apache/oozie/util/db/OperationRetryHandler.java Lines 101 (patched) <https://reviews.apache.org/r/60544/#comment254382> nit: sleepBeforeRetry or something would be more expressive / better describe what this function does. core/src/main/java/org/apache/oozie/util/db/OperationRetryHandler.java Lines 102 (patched) <https://reviews.apache.org/r/60544/#comment254377> e is never used core/src/main/java/org/apache/oozie/util/db/PersistenceExceptionSubclassFilterRetryPredicate.java Lines 24 (patched) <https://reviews.apache.org/r/60544/#comment254344> Do not use wildcard import. core/src/test/java/org/apache/oozie/test/XTestCase.java Lines 872 (patched) <https://reviews.apache.org/r/60544/#comment254323> What is the added value of ``E extends Object`` here? E already extends Object. minitest/src/test/java/org/apache/oozie/test/WorkflowTest.java Lines 109 (patched) <https://reviews.apache.org/r/60544/#comment254325> there is no need to divide by 1000 to determine if second is even minitest/src/test/java/org/apache/oozie/test/WorkflowTest.java Lines 112 (patched) <https://reviews.apache.org/r/60544/#comment254326> this is not necessary - Attila Sasvari On July 3, 2017, 3:15 p.m., András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60544/ > ----------------------------------------------------------- > > (Updated July 3, 2017, 3:15 p.m.) > > > Review request for oozie, Attila Sasvari, Peter Cseh, and Peter Bacsko. > > > Repository: oozie-git > > > Description > ------- > > https://issues.apache.org/jira/browse/OOZIE-2854 > > > Diffs > ----- > > core/pom.xml acddf349a89cf09a7fc4f384ebcaec56dfd0ab48 > > core/src/main/java/org/apache/oozie/executor/jpa/JsonBeanPersisterExecutor.java > PRE-CREATION > core/src/main/java/org/apache/oozie/executor/jpa/QueryExecutor.java > 8d94c23e40d1281864db40e141b200ca207a6324 > core/src/main/java/org/apache/oozie/service/JPAService.java > 028381d3b72bcc3b8c2cd27cacb3e0ac6d48d146 > core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java > cfe1522a4b1f89085eb29e7f1281c2abd631bdc2 > core/src/main/java/org/apache/oozie/store/WorkflowStore.java > c565e74893b863caef6c93015cfe38fe520d04ec > core/src/main/java/org/apache/oozie/util/db/BasicDataSourceWrapper.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/DatabaseRetryPredicate.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/FailingMySQLDriverWrapper.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/OperationRetryHandler.java > PRE-CREATION > > core/src/main/java/org/apache/oozie/util/db/PersistenceExceptionSubclassFilterRetryPredicate.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/RetryAttemptCounter.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/RuntimeExceptionInjector.java > PRE-CREATION > core/src/main/resources/META-INF/persistence.xml > bad9278597fcd4f93b4cc482afae8af14beaa922 > core/src/main/resources/oozie-default.xml > c60a4581a84d4c67a1ac1cf3dfdc252b85ccd01c > core/src/main/resources/oozie-log4j.properties > c065f3cd4c5a3df1308b69d7c16e8fcfa8796efc > core/src/test/java/org/apache/oozie/test/XTestCase.java > 161927ac8f1132b3080d2924844826fcc7b807a5 > > core/src/test/java/org/apache/oozie/util/db/TestOozieDmlStatementPredicate.java > PRE-CREATION > core/src/test/java/org/apache/oozie/util/db/TestOperationRetryHandler.java > PRE-CREATION > > core/src/test/java/org/apache/oozie/util/db/TestPersistenceExceptionSubclassFilterRetryPredicate.java > PRE-CREATION > core/src/test/java/org/apache/oozie/util/db/TestRetryAttemptCounter.java > PRE-CREATION > minitest/pom.xml 9515284bb5f32c279a93161c10e6571680e4f9fc > > minitest/src/test/java/org/apache/oozie/test/TestParallelJPAOperationRetries.java > PRE-CREATION > minitest/src/test/java/org/apache/oozie/test/TestWorkflowRetries.java > PRE-CREATION > minitest/src/test/java/org/apache/oozie/test/WorkflowTest.java > 2845f0af6efb9ef75fdbfcb326115c62e6fb3bdd > minitest/src/test/resources/hsqldb-oozie-site.xml > fa5fe9c3185e973e8247d7bf10b126119d9c02c9 > minitest/src/test/resources/oozie-log4j.properties > c142d725140930bfa89cd2b163d0768a4c3a750a > minitest/src/test/resources/parallel-fs-and-shell.xml PRE-CREATION > minitest/src/test/resources/wf-test.xml > 20c4946862039a65c76ed7f49991345e90a694de > pom.xml 16c5137d44d7db891da46f80adb51c85e4c1b214 > > > Diff: https://reviews.apache.org/r/60544/diff/2/ > > > Testing > ------- > > Tests covered in code: > > Unit tests > ========== > > * testing the retry handler, the retry predicate filter, and parallel calls > to JPA `EntityManager` (mostly Oozie database reads and writes) when > injecting failures > > Integration tests > ================= > > * using the `MiniOozieTestCase` framework > * fixing it so that also asynchronous workflow applications (the ones that > use `CallableQueueService`) can be run > * following workflow scenarios: > * a very simple one consisting only of a `<start/>` and an `<end/>` node > * a more sophisticated one consisting of multiple synchronous `<fs/>` nodes > and a `<decision/>` node > * the ultimate one consisting of a `<decision/>` node, and two branches of an > `<fs/>` and an asynchronous `<shell/>` nodes > > Test cases run: > ``` > mvn clean test > -Dtest=TestOperationRetryHandler,TestPersistenceExceptionSubclassFilterRetryPredicate,TestParallelJPAOperationRetries,TestWorkflow,TestWorkflowRetries,TestJPAService,TestRetryAttemptCounter > ``` > > Functional and stress tests performed on a 4-node MySQL cluster. MySQL daemon > has been stopped / killed / restarted several times. Also firewall rules have > been modified temporarily to simulate network outages. > > > Thanks, > > András Piros > >
