----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60544/#review179358 -----------------------------------------------------------
core/pom.xml Lines 42 (patched) <https://reviews.apache.org/r/60544/#comment254019> White space? core/src/main/java/org/apache/oozie/ErrorCode.java Lines 110 (patched) <https://reviews.apache.org/r/60544/#comment254020> You can delete this. Originally this had a purporse but not anymore. core/src/main/java/org/apache/oozie/service/JPAService.java Lines 267 (patched) <https://reviews.apache.org/r/60544/#comment254022> Do we need this? Usually this is done only once, when Oozie starts. If there's a problem during startup IMO it's good to know about it and fail. Let's see other people's opinion. core/src/main/java/org/apache/oozie/util/db/DBOperationRetryHandler.java Lines 44 (patched) <https://reviews.apache.org/r/60544/#comment254034> This is not relevant anymore core/src/main/java/org/apache/oozie/util/db/DBOperationRetryHandler.java Lines 84 (patched) <https://reviews.apache.org/r/60544/#comment254023> Not sure if we have to log this on ERROR level. core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java Lines 43 (patched) <https://reviews.apache.org/r/60544/#comment254024> Can't we move this under src/test ? core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java Lines 157-161 (patched) <https://reviews.apache.org/r/60544/#comment254026> It's cleaner if you add all these literals to sets then perform the startsWith() and contains() operations on the set elements in a for loop. core/src/main/java/org/apache/oozie/util/db/MySQLRetryPredicate.java Lines 27 (patched) <https://reviews.apache.org/r/60544/#comment254035> Do we still need this class? core/src/main/java/org/apache/oozie/util/db/NestedRetryUtil.java Lines 26 (patched) <https://reviews.apache.org/r/60544/#comment254033> Let's add comments here and explain *properly* what this class does & why. core/src/main/java/org/apache/oozie/util/db/PersistenceExceptionSubclassFilterRetryPredicate.java Lines 29 (patched) <https://reviews.apache.org/r/60544/#comment254028> Does this work properly? The exception that we receive is always JPAExecutorException so these exceptions has to be extracted from it. I don't see any logic (or calls to getAllExceptions()) to do that. core/src/main/java/org/apache/oozie/util/db/PersistenceExceptionSubclassFilterRetryPredicate.java Lines 34-68 (patched) <https://reviews.apache.org/r/60544/#comment254027> Create a Set<Class<?>> with all the classes mentioned here and process it accordingly in a loop to have smaller code. core/src/main/java/org/apache/oozie/util/db/RuntimeExceptionInjector.java Lines 28 (patched) <https://reviews.apache.org/r/60544/#comment254029> Another test code - can we move this to src/test ? core/src/main/java/org/apache/oozie/util/db/RuntimeExceptionInjector.java Lines 43 (patched) <https://reviews.apache.org/r/60544/#comment254030> I don't think we need such a generic logic to throw exceptions or do we? For testing purposes, it's more than enough to throw a specific exception I guess. core/src/test/java/org/apache/oozie/test/XTestCase.java Lines 872 (patched) <https://reviews.apache.org/r/60544/#comment254031> Nice refactor:) minitest/src/test/java/org/apache/oozie/test/TestParallelJPAOperationRetries.java Lines 37 (patched) <https://reviews.apache.org/r/60544/#comment254032> Could you add a short comment that describes what we test here exactly? - Peter Bacsko On jún. 29, 2017, 4:35 du, András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60544/ > ----------------------------------------------------------- > > (Updated jún. 29, 2017, 4:35 du) > > > 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/ErrorCode.java > 662e1edc9c4b23b3606c751bf5ed4b531ee7ac62 > > 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/DBOperationRetryHandler.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/MySQLRetryPredicate.java > PRE-CREATION > core/src/main/java/org/apache/oozie/util/db/NestedRetryUtil.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/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/TestDBOperationRetryHandler.java > PRE-CREATION > > core/src/test/java/org/apache/oozie/util/db/TestPersistenceExceptionSubclassFilterRetryPredicate.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/1/ > > > 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=TestDBOperationRetryHandler,TestPersistenceExceptionSubclassFilterRetryPredicate,TestParallelJPAOperationRetries,TestWorkflow,TestWorkflowRetries,TestJPAService > ``` > > 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 > >
