----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65481/#review196707 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java Lines 28 (patched) <https://reviews.apache.org/r/65481/#comment276499> Would use oozie.sql.failure.percent core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java Line 32 (original), 34 (patched) <https://reviews.apache.org/r/65481/#comment276501> Would have int failurePercent in case it never should be null. core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java Lines 36-39 (patched) <https://reviews.apache.org/r/65481/#comment276500> Usage of System#getProperty(String, String) would be much cleaner: https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#getProperty-java.lang.String-java.lang.String- core/src/main/java/org/apache/oozie/util/db/FailingMySQLDriverWrapper.java Lines 43 (patched) <https://reviews.apache.org/r/65481/#comment276502> Why have it on two different places? Making one of the constants w/ the same name public, and reusing them any other times. core/src/main/java/org/apache/oozie/util/db/RuntimeExceptionInjector.java Lines 46-53 (patched) <https://reviews.apache.org/r/65481/#comment276503> Would extract method refreshFailurePercent(), as well use https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#getProperty-java.lang.String-java.lang.String- But even better, I'd rather create three of this: - FailingHSQLDBDriverWrapper - NeverFailingHSQLDBDriverWrapper - AlwaysFailingHSQLDBDriverWrapper and reset this Configuration property for each of the unit tests. core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java Lines 80 (patched) <https://reviews.apache.org/r/65481/#comment276504> Would rather create a NeverFailingHSQLDBDriverWrapper with this default 0 failure percent, but wouldn't have this constant here anyway. core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java Lines 1109 (patched) <https://reviews.apache.org/r/65481/#comment276505> Would instead create an AlwaysFailingHSQLDBDriverWrapper and reuse here. To set and maybe forget to reset :) system properties across unit tests isn't a good idea, since all the other tests running in the same JVM after setting and maybe not resetting the system property would behave unexpectedly. core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java Lines 1147 (patched) <https://reviews.apache.org/r/65481/#comment276506> Would instead create an AlwaysFailingHSQLDBDriverWrapper and reuse here. To set and maybe forget to reset :) system properties across unit tests isn't a good idea, since all the other tests running in the same JVM after setting and maybe not resetting the system property would behave unexpectedly. core/src/test/resources/hsqldb-oozie-site.xml Line 23 (original), 23 (patched) <https://reviews.apache.org/r/65481/#comment276507> Sure we always want to use that in all our JUnit tests? I'd rather create three of this: - FailingHSQLDBDriverWrapper - NeverFailingHSQLDBDriverWrapper - AlwaysFailingHSQLDBDriverWrapper and reset this Configuration property for each of the unit tests. - András Piros On Feb. 2, 2018, 1:33 p.m., Kinga Marton wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65481/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2018, 1:33 p.m.) > > > Review request for oozie, András Piros and Attila Sasvari. > > > Repository: oozie-git > > > Description > ------- > > Upon SLACalculatorMemory.addRegistration, Oozie puts an entry into an > in-memory concurrent hashmap ("slaMap"), and later persists it in the Oozie > database. > However, if there is a failure during the database operation, a > JPAExecutorException is thrown, and the entry is not removed from the SLA map. > It may introduce inconsistency between the Oozie database and the SLA map. > To prevent this, a rollback mechanism (with proper logging) should be > implemented. It would also make sense to do more sanity/consistency check in > the Oozie server. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java ef019e73 > core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java > 0e310253 > core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java > fe9f08b1 > core/src/main/java/org/apache/oozie/util/db/FailingMySQLDriverWrapper.java > f0e2b181 > core/src/main/java/org/apache/oozie/util/db/RuntimeExceptionInjector.java > 2b55e858 > core/src/test/java/org/apache/oozie/service/TestConfigurationService.java > 1b68090d > core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java > ee906f45 > core/src/test/resources/hsqldb-oozie-site.xml fa5fe9c3 > > > Diff: https://reviews.apache.org/r/65481/diff/1/ > > > Testing > ------- > > > Thanks, > > Kinga Marton > >
