> On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > core/pom.xml > > Lines 39 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768464#file1768464line39> > > > > Do not add this dependency twice: > > > > https://github.com/apache/oozie/blob/378f294c2cd92ac0d505201857f03746ce2e58ac/core/pom.xml#L743
Setting it to `compile` scope because we need `FailingMySQLDriverWrapper` to work. > On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > core/src/main/java/org/apache/oozie/service/JPAService.java > > Line 413 (original), 496 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768467#file1768467line499> > > > > Use ``&& !updateQueryList.isEmpty()`` instead of size() >0 Using `CollectionUtils.isNotEmpty()`. > On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java > > Lines 71 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768468#file1768468line71> > > > > 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. Good idea, there is a lot more to do when speaking of constans / named queries / other JPA primitives. Will fire up a separate JIRA. > On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > core/src/main/java/org/apache/oozie/util/db/BasicDataSourceWrapper.java > > Lines 41 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768470#file1768470line41> > > > > 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. I would keep the fix in line w/ [the original DBCP one](https://github.com/apache/commons-dbcp/blob/DBCP_1_4_x_BRANCH/src/java/org/apache/commons/dbcp/BasicDataSource.java#L1588-L1660). > On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > core/src/main/java/org/apache/oozie/util/db/BasicDataSourceWrapper.java > > Lines 42 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768470#file1768470line42> > > > > 1 try with multiple catch block should be enough I would keep the fix in line w/ [the original DBCP one](https://github.com/apache/commons-dbcp/blob/DBCP_1_4_x_BRANCH/src/java/org/apache/commons/dbcp/BasicDataSource.java#L1588-L1660). > On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java > > Lines 347 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768472#file1768472line347> > > > > nit: OozieDBCLI also uses the tablenames. We could extract these into a > > central place perhaps in a separate Jira? Good idea, there is a lot more to do when speaking of constans / named queries / other JPA primitives. Will fire up a separate JIRA. > On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > core/src/main/java/org/apache/oozie/util/db/OperationRetryHandler.java > > Lines 31 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768475#file1768475line31> > > > > 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. Excerpt from the Javadoc of the class `com.google.common.annotations.VisibleForTesting`: ``` An annotation that indicates that the visibility of a type or member has been relaxed to make the code testable. ``` So the field `OperationRetryHandler.NESTED_RETRY_ATTEMPT_COUNTER` would be private, but for sake of testability its visibility is relaxed to package protected - hence the annotation. > On July 4, 2017, 1:16 p.m., Attila Sasvari wrote: > > minitest/src/test/java/org/apache/oozie/test/WorkflowTest.java > > Lines 112 (patched) > > <https://reviews.apache.org/r/60544/diff/2/?file=1768490#file1768490line114> > > > > this is not necessary >From an IDE(A) perspective, it's good to see why your test actually breaks. >And this `assumeFalse()` makes the root cause visible. From Maven, yes, >`JAVA_HOME` can be considered as always set, so no issues there. Without adding the additional workflow property having `JAVA_HOME` as value, the test will fail in an IDE(A): ``` junit.framework.AssertionFailedError: Expected :SUCCEEDED Actual :RUNNING <Click to see difference> at junit.framework.Assert.fail(Assert.java:57) at junit.framework.Assert.failNotEquals(Assert.java:329) at junit.framework.Assert.assertEquals(Assert.java:78) at junit.framework.Assert.assertEquals(Assert.java:86) at junit.framework.TestCase.assertEquals(TestCase.java:253) at org.apache.oozie.test.TestWorkflow.runWorkflowFromFile(TestWorkflow.java:167) at org.apache.oozie.test.TestWorkflow.testParallelFsAndShellWorkflowCompletesSuccessfully(TestWorkflow.java:117) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at junit.framework.TestCase.runTest(TestCase.java:176) at junit.framework.TestCase.runBare(TestCase.java:141) at junit.framework.TestResult$1.protect(TestResult.java:122) at junit.framework.TestResult.runProtected(TestResult.java:142) at junit.framework.TestResult.run(TestResult.java:125) at junit.framework.TestCase.run(TestCase.java:129) at junit.framework.TestSuite.runTest(TestSuite.java:255) at junit.framework.TestSuite.run(TestSuite.java:250) at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:84) at org.junit.runner.JUnitCore.run(JUnitCore.java:160) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68) at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70) ``` - András ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60544/#review179556 ----------------------------------------------------------- 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 > >
