Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-03-23 Thread András Piros via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/#review199865
---


Ship it!




Ship It!

- András Piros


On March 23, 2018, 7:28 a.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65481/
> ---
> 
> (Updated March 23, 2018, 7:28 a.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/AlwaysFailingHSQLDriverMapper.java
>  PRE-CREATION 
>   core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java 
> 0e310253 
>   core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java 
> PRE-CREATION 
>   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/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java 
> ee906f45 
> 
> 
> Diff: https://reviews.apache.org/r/65481/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-03-23 Thread Kinga Marton via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/
---

(Updated March 23, 2018, 7:28 a.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 (updated)
-

  core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java ef019e73 
  
core/src/main/java/org/apache/oozie/util/db/AlwaysFailingHSQLDriverMapper.java 
PRE-CREATION 
  core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java 
0e310253 
  core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java 
PRE-CREATION 
  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/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java ee906f45 


Diff: https://reviews.apache.org/r/65481/diff/6/

Changes: https://reviews.apache.org/r/65481/diff/5-6/


Testing
---


Thanks,

Kinga Marton

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-03-22 Thread András Piros via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/#review199790
---




core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1129 (patched)


Extract method w/ name as the comment suggests, and drop the comment.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1137 (patched)


Please use `Assert.assertEquals()` that features a `String message`, and 
have your comment as the `message`. This way, comment will be part of code - 
can't rot!



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1147-1151 (patched)


Extract method and drop comment.


- András Piros


On March 22, 2018, 4:44 p.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65481/
> ---
> 
> (Updated March 22, 2018, 4:44 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/AlwaysFailingHSQLDriverMapper.java
>  PRE-CREATION 
>   core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java 
> 0e310253 
>   core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java 
> PRE-CREATION 
>   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/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java 
> ee906f45 
> 
> 
> Diff: https://reviews.apache.org/r/65481/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-03-22 Thread Kinga Marton via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/
---

(Updated March 22, 2018, 4:44 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 (updated)
-

  core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java ef019e73 
  
core/src/main/java/org/apache/oozie/util/db/AlwaysFailingHSQLDriverMapper.java 
PRE-CREATION 
  core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java 
0e310253 
  core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java 
PRE-CREATION 
  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/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java ee906f45 


Diff: https://reviews.apache.org/r/65481/diff/5/

Changes: https://reviews.apache.org/r/65481/diff/4-5/


Testing
---


Thanks,

Kinga Marton

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-03-19 Thread András Piros via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/#review199405
---




core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java
Lines 27-30 (patched)


If it's visible because of testing, please add `@VisibleForTesting`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1095 (patched)


Please add a more self-descriptive name like 
`testWhenSLARegistrationIsAddedBeanIsStoredCorrectly()`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1104 (patched)


Please add a more self-descriptive name like 
`testWhenSLARegistrationIsAddedAndAllDBCallsAreDisruptedBeanIsNotStored()`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1123 (patched)


Please add a more self-descriptive name like 
`testWhenSLARegistrationIsUpdatedBeanIsStoredCorrectly()`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1141 (patched)


Please add a more self-descriptive name like 
`testWhenSLARegistrationIsUpdatedAndAllDBCallsAreDisruptedBeanIsNotStored()`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1179 (patched)


Use a more descriptive name as `SLARegistrationDmlPredicate`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1185 (patched)


Please use Guava's 
[`Strings#isNullOrEmpty()`](https://google.github.io/guava/releases/23.0/api/docs/com/google/common/base/Strings.html#isNullOrEmpty-java.lang.String-)
 instead.


- András Piros


On Feb. 19, 2018, 2:14 p.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65481/
> ---
> 
> (Updated Feb. 19, 2018, 2:14 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/AlwaysFailingHSQLDriverMapper.java
>  PRE-CREATION 
>   core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java 
> 0e310253 
>   core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java 
> PRE-CREATION 
>   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/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java 
> ee906f45 
> 
> 
> Diff: https://reviews.apache.org/r/65481/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-19 Thread Kinga Marton via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/
---

(Updated Feb. 19, 2018, 2:14 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 (updated)
-

  core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java ef019e73 
  
core/src/main/java/org/apache/oozie/util/db/AlwaysFailingHSQLDriverMapper.java 
PRE-CREATION 
  core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java 
0e310253 
  core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java 
PRE-CREATION 
  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/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java ee906f45 


Diff: https://reviews.apache.org/r/65481/diff/4/

Changes: https://reviews.apache.org/r/65481/diff/3-4/


Testing
---


Thanks,

Kinga Marton

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-13 Thread Kinga Marton via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/
---

(Updated Feb. 13, 2018, 3:19 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 (updated)
-

  core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java ef019e73 
  
core/src/main/java/org/apache/oozie/util/db/AlwaysFailingHSQLDriverMapper.java 
PRE-CREATION 
  core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java 
0e310253 
  core/src/main/java/org/apache/oozie/util/db/FailingDBHelperForTest.java 
PRE-CREATION 
  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/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java ee906f45 


Diff: https://reviews.apache.org/r/65481/diff/3/

Changes: https://reviews.apache.org/r/65481/diff/2-3/


Testing
---


Thanks,

Kinga Marton

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-13 Thread Kinga Marton via Review Board 


> On Feb. 6, 2018, 4:15 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/util/db/ThreadVariables.java
> > Lines 23-37 (patched)
> > 
> >
> > I don't see calling `setDbPredicate()` or `unsetDbPredicate()` from 
> > anywhere else than test code.
> > 
> > When using `ThreadLocal` we have to be very careful to unset after 
> > every use to avoid resource / memory leaks.
> > 
> > Generally, I think it would be a better idea to inject a new DB 
> > predicate by other means - I would only go for `ThreadLocal` usage when 
> > most of the time I need different `dbPredicate` to every `Thread` - which 
> > is not the case here.
> > 
> > Let's think a bit more on `Predicate` injecting scenarios apart from 
> > using `ThreadLocal`.

My next ideea is what Peti suggested: a helper class with a static field. Is 
not a very elegant solution. Do you have some better ideeas?


- Kinga


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/#review196897
---


On Feb. 6, 2018, 2:43 p.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65481/
> ---
> 
> (Updated Feb. 6, 2018, 2:43 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/AlwaysFailingHSQLDriverMapper.java
>  PRE-CREATION 
>   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/ThreadVariables.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java 
> ee906f45 
> 
> 
> Diff: https://reviews.apache.org/r/65481/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-06 Thread Peter Bacsko via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/#review196901
---




core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java
Line 58 (original), 56 (patched)


Using Predicate like that creates a raw-type warning



core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
Lines 34 (patched)


super() is implicit, not needed to call directly



core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
Lines 40 (patched)


Predicate is generic interface. I believe this code creates an unchecked 
raw-type warning.



core/src/main/java/org/apache/oozie/util/db/ThreadVariables.java
Lines 23-37 (patched)


Agree with Andras. Using ThreadLocal is very confusing here. If it's not 
possible to reload/re-instantiate the class that uses this predicate, we're 
still better off with a simple helper class that has a static Predicate field 
which can be set/unset.



core/src/main/java/org/apache/oozie/util/db/ThreadVariables.java
Lines 24 (patched)


Same here about Predicate, it should be Predicate or Predicate 
depending on your needs.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1176 (patched)


Try conf.setInt()


- Peter Bacsko


On febr. 6, 2018, 2:43 du, Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65481/
> ---
> 
> (Updated febr. 6, 2018, 2:43 du)
> 
> 
> 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/AlwaysFailingHSQLDriverMapper.java
>  PRE-CREATION 
>   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/ThreadVariables.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java 
> ee906f45 
> 
> 
> Diff: https://reviews.apache.org/r/65481/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-06 Thread András Piros via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/#review196897
---



Several comments addressing mostly the original scope of 
[OOZIE-3134](https://issues.apache.org/jira/browse/OOZIE-3134). As Rohini 
pointed out, this JIRA could be extended by 
[OOZIE-1980](https://issues.apache.org/jira/browse/OOZIE-1980) functionality. 
Please wait for her response before going on with the fix.


core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java
Line 58 (original), 56 (patched)


`@Nullable final Predicate predicate`



core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java
Lines 59 (patched)


Could go to `else` path.



core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
Lines 40 (patched)


I don't see calling `setDbPredicate()` or `unsetDbPredicate()` from 
anywhere else than test code.

When using `ThreadLocal` we have to be very careful to unset after every 
use to avoid resource / memory leaks.



core/src/main/java/org/apache/oozie/util/db/ThreadVariables.java
Lines 23-37 (patched)


I don't see calling `setDbPredicate()` or `unsetDbPredicate()` from 
anywhere else than test code.

When using `ThreadLocal` we have to be very careful to unset after every 
use to avoid resource / memory leaks.

Generally, I think it would be a better idea to inject a new DB predicate 
by other means - I would only go for `ThreadLocal` usage when most of the time 
I need different `dbPredicate` to every `Thread` - which is not the case here.

Let's think a bit more on `Predicate` injecting scenarios apart from using 
`ThreadLocal`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1109-1110 (patched)


Inside `try`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines  (patched)


Not needed when `fail(...)` is called from `catch`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1115 (patched)


Not needed when `fail(...)` is called from `catch`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1120 (patched)


Not needed when `fail(...)` is called from `catch`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1154-1155 (patched)


Both should be part of `try`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1156 (patched)


Not needed when `fail(...)` is called from `catch`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1161 (patched)


Better call `fail(...)` here.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1166 (patched)


Not needed when `fail(...)` is called from `catch`.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1173 (patched)


`Boolean.TRUE.toString()`



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1175 (patched)


Use `AlwaysFailingHSQLDBDriverMapper.class.getCanonicalName()` and we don't 
have to modify string values when renaming / moving.



core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1183 (patched)


`OPERATIONS`


- András Piros


On Feb. 6, 2018, 2:43 p.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65481/
> ---
> 
> (Updated Feb. 6, 2018, 2:43 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

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-06 Thread Kinga Marton via Review Board 


> On Feb. 2, 2018, 2:09 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
> > Lines 36-39 (patched)
> > 
> >
> > 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-

With the AlwaysFailingdriverMapper, I will not need this system property anymore


> On Feb. 2, 2018, 2:09 p.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/util/db/RuntimeExceptionInjector.java
> > Lines 46-53 (patched)
> > 
> >
> > 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.

I have created the AlwaysFailingHSQLDBDriverWrapper, but I skipped the creation 
of NeverFailingHSQLDBDriverWrapper, because the defaut jdbdDriver is actually a 
"Never failing" one.


- Kinga


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/#review196707
---


On Feb. 6, 2018, 2:43 p.m., Kinga Marton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65481/
> ---
> 
> (Updated Feb. 6, 2018, 2:43 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/AlwaysFailingHSQLDriverMapper.java
>  PRE-CREATION 
>   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/ThreadVariables.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java 
> ee906f45 
> 
> 
> Diff: https://reviews.apache.org/r/65481/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kinga Marton
> 
>

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-06 Thread Kinga Marton via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/
---

(Updated Feb. 6, 2018, 2:43 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 (updated)
-

  core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java ef019e73 
  
core/src/main/java/org/apache/oozie/util/db/AlwaysFailingHSQLDriverMapper.java 
PRE-CREATION 
  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/ThreadVariables.java PRE-CREATION 
  core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java ee906f45 


Diff: https://reviews.apache.org/r/65481/diff/2/

Changes: https://reviews.apache.org/r/65481/diff/1-2/


Testing
---


Thanks,

Kinga Marton

Re: Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-02 Thread András Piros via Review Board 

---
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)


Would use oozie.sql.failure.percent



core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
Line 32 (original), 34 (patched)


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)


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)


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)


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)


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)


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)


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)


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 
>

Review Request 65481: OOZIE-3134 - Potential inconsistency between the in-memory SLA map and the Oozie database

2018-02-02 Thread Kinga Marton via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65481/
---

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