You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Kinga Marton via Review Board <no...@reviews.apache.org> on 2018/02/02 13:33:17 UTC
Review Request 65481: OOZIE-3134 - Potential inconsistency between the
in-memory SLA map and the Oozie database
-----------------------------------------------------------
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
Re: Review Request 65481: OOZIE-3134 - Potential inconsistency
between the in-memory SLA map and the Oozie database
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65481/diff/1/?file=1952234#file1952234line36>
> >
> > 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)
> > <https://reviews.apache.org/r/65481/diff/1/?file=1952236#file1952236line46>
> >
> > 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
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 65481: OOZIE-3134 - Potential inconsistency
between the in-memory SLA map and the Oozie database
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65481/#comment276868>
Using Predicate like that creates a raw-type warning
core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
Lines 34 (patched)
<https://reviews.apache.org/r/65481/#comment276858>
super() is implicit, not needed to call directly
core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
Lines 40 (patched)
<https://reviews.apache.org/r/65481/#comment276863>
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)
<https://reviews.apache.org/r/65481/#comment276860>
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)
<https://reviews.apache.org/r/65481/#comment276865>
Same here about Predicate, it should be Predicate<T> or Predicate<?> depending on your needs.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1176 (patched)
<https://reviews.apache.org/r/65481/#comment276862>
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
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> 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)
> > <https://reviews.apache.org/r/65481/diff/2/?file=1953367#file1953367line23>
> >
> > 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
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65481/#comment276826>
`@Nullable final Predicate predicate`
core/src/main/java/org/apache/oozie/util/db/FailingConnectionWrapper.java
Lines 59 (patched)
<https://reviews.apache.org/r/65481/#comment276829>
Could go to `else` path.
core/src/main/java/org/apache/oozie/util/db/FailingHSQLDBDriverWrapper.java
Lines 40 (patched)
<https://reviews.apache.org/r/65481/#comment276848>
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)
<https://reviews.apache.org/r/65481/#comment276850>
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)
<https://reviews.apache.org/r/65481/#comment276851>
Inside `try`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1111 (patched)
<https://reviews.apache.org/r/65481/#comment276852>
Not needed when `fail(...)` is called from `catch`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1115 (patched)
<https://reviews.apache.org/r/65481/#comment276853>
Not needed when `fail(...)` is called from `catch`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1120 (patched)
<https://reviews.apache.org/r/65481/#comment276854>
Not needed when `fail(...)` is called from `catch`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1154-1155 (patched)
<https://reviews.apache.org/r/65481/#comment276844>
Both should be part of `try`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1156 (patched)
<https://reviews.apache.org/r/65481/#comment276846>
Not needed when `fail(...)` is called from `catch`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1161 (patched)
<https://reviews.apache.org/r/65481/#comment276843>
Better call `fail(...)` here.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1166 (patched)
<https://reviews.apache.org/r/65481/#comment276845>
Not needed when `fail(...)` is called from `catch`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1173 (patched)
<https://reviews.apache.org/r/65481/#comment276838>
`Boolean.TRUE.toString()`
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1175 (patched)
<https://reviews.apache.org/r/65481/#comment276824>
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)
<https://reviews.apache.org/r/65481/#comment276839>
`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 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
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65481/#comment280240>
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)
<https://reviews.apache.org/r/65481/#comment280239>
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)
<https://reviews.apache.org/r/65481/#comment280241>
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
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/65481/#comment279728>
If it's visible because of testing, please add `@VisibleForTesting`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1095 (patched)
<https://reviews.apache.org/r/65481/#comment279729>
Please add a more self-descriptive name like `testWhenSLARegistrationIsAddedBeanIsStoredCorrectly()`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1104 (patched)
<https://reviews.apache.org/r/65481/#comment279730>
Please add a more self-descriptive name like `testWhenSLARegistrationIsAddedAndAllDBCallsAreDisruptedBeanIsNotStored()`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1123 (patched)
<https://reviews.apache.org/r/65481/#comment279731>
Please add a more self-descriptive name like `testWhenSLARegistrationIsUpdatedBeanIsStoredCorrectly()`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1141 (patched)
<https://reviews.apache.org/r/65481/#comment279732>
Please add a more self-descriptive name like `testWhenSLARegistrationIsUpdatedAndAllDBCallsAreDisruptedBeanIsNotStored()`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1179 (patched)
<https://reviews.apache.org/r/65481/#comment279733>
Use a more descriptive name as `SLARegistrationDmlPredicate`.
core/src/test/java/org/apache/oozie/sla/TestSLACalculatorMemory.java
Lines 1185 (patched)
<https://reviews.apache.org/r/65481/#comment279734>
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
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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