You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Purshotam Shah <pu...@yahoo-inc.com> on 2016/05/25 18:45:43 UTC
Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work for
few cases
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/
-----------------------------------------------------------
Review request for oozie.
Bugs: OOZIE-2501
https://issues.apache.org/jira/browse/OOZIE-2501
Repository: oozie-git
Description
-------
OOZIE-2501 ZK reentrant lock doesn't work for few cases
Diffs
-----
core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 2939b604da579adeca399c879425eafab2536b88
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
core/src/main/java/org/apache/oozie/service/ZKLocksService.java 35fc8a6592b2ba3c5ed4a78b9cc6ed90dd417f40
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 70aa4d7d54c286fec1858a0d0f97f09fff9b4b9f
Diff: https://reviews.apache.org/r/47837/diff/
Testing
-------
Thanks,
Purshotam Shah
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review144690
-----------------------------------------------------------
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 29)
<https://reviews.apache.org/r/47837/#comment210728>
Keep in mind that I'm going to make heavy changes in this class, you might want to incorporate them.
See: https://issues.apache.org/jira/browse/OOZIE-2584
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 339)
<https://reviews.apache.org/r/47837/#comment210730>
Why two distinct variables?
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 342)
<https://reviews.apache.org/r/47837/#comment210733>
extract magic number to a constant
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 350)
<https://reviews.apache.org/r/47837/#comment210729>
I understand the purpose of this, but System.gc() is very dodgy especially in a test.
I think if you want to test the fact that you use a Guava-based map with weak values, you're better off checking types. Guava-specific map is MapMakerInternalMap and the field that determines the "strength" of the values is valueStrength. It's not nice to access a package-private field with reflection but I think it's more acceptable and predictable than System.gc().
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 372)
<https://reviews.apache.org/r/47837/#comment210734>
Why does the method start with an underscore? Because of JUnit? Could be just simply checkLockRelease().
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java (line 30)
<https://reviews.apache.org/r/47837/#comment210737>
This class suffers from the same problem as TestMemoryLocks, except that tests sleep for 1000ms which makes them less prone to errors. We might want to create a new JIRA to fix all these Thread.sleep() stuff.
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java (line 524)
<https://reviews.apache.org/r/47837/#comment210735>
See my comments in TestMemoryLocksService
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java (line 553)
<https://reviews.apache.org/r/47837/#comment210738>
Again, method naming
- Peter Bacsko
On aug. 3, 2016, 4:56 du, Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated aug. 3, 2016, 4:56 du)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Rohini Palaniswamy <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review144632
-----------------------------------------------------------
Ship it!
Ship It!
- Rohini Palaniswamy
On Aug. 3, 2016, 4:56 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2016, 4:56 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/
-----------------------------------------------------------
(Updated Sept. 22, 2016, 5:55 p.m.)
Review request for oozie.
Bugs: OOZIE-2501
https://issues.apache.org/jira/browse/OOZIE-2501
Repository: oozie-git
Description
-------
OOZIE-2501 ZK reentrant lock doesn't work for few cases
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java f0a87e54162200e36168539a75281675f2fd6358
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
Diff: https://reviews.apache.org/r/47837/diff/
Testing
-------
Thanks,
Purshotam Shah
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
> On Aug. 8, 2016, 11:58 a.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java, line 352
> > <https://reviews.apache.org/r/47837/diff/4/?file=1461452#file1461452line352>
> >
> > Yes I understand that you want to make sure that weakrefs are cleared up after a minor GC.
> >
> > However System.gc() makes no guarantees about when/how it runs. It might work just fine in the current JDK, but what if something changes in the next version? The the test might become flaky. Also someone might come along and disable explicit GC for some reason (unlikely, but can happen).
> >
> > Unfortunately there's no super-reliable way to test weakrefs, that's why I recommended checking the types - if the types are OK, we know that we're using Guava and we can trust it (eg. we can trust the fact that if the strenght is "weak", we're using weak refs so it will work as expected). We're not testing Guava functionality this way. If the assertion fails, it will fail consistently and we know that something has changed.
> >
> > But I'm not super-religious about it - it's just the way how I would do this kind of thing. The final decision is yours :)
I run in a loop, and I didn't see any issue. Anyway it's just test case, so let keep it for now and if its become flaky, we can look it later on
> On Aug. 8, 2016, 11:58 a.m., Peter Bacsko wrote:
> > core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java, line 374
> > <https://reviews.apache.org/r/47837/diff/4/?file=1461452#file1461452line374>
> >
> > OK, I'll leave this to you :)
I will fix it.
- Purshotam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review145089
-----------------------------------------------------------
On Aug. 3, 2016, 4:56 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2016, 4:56 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review145089
-----------------------------------------------------------
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 342)
<https://reviews.apache.org/r/47837/#comment211280>
The value "5000" is repeated many times. It's better to extract it to a final variable.
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 350)
<https://reviews.apache.org/r/47837/#comment211284>
Yes I understand that you want to make sure that weakrefs are cleared up after a minor GC.
However System.gc() makes no guarantees about when/how it runs. It might work just fine in the current JDK, but what if something changes in the next version? The the test might become flaky. Also someone might come along and disable explicit GC for some reason (unlikely, but can happen).
Unfortunately there's no super-reliable way to test weakrefs, that's why I recommended checking the types - if the types are OK, we know that we're using Guava and we can trust it (eg. we can trust the fact that if the strenght is "weak", we're using weak refs so it will work as expected). We're not testing Guava functionality this way. If the assertion fails, it will fail consistently and we know that something has changed.
But I'm not super-religious about it - it's just the way how I would do this kind of thing. The final decision is yours :)
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 372)
<https://reviews.apache.org/r/47837/#comment211285>
OK, I'll leave this to you :)
- Peter Bacsko
On aug. 3, 2016, 4:56 du, Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated aug. 3, 2016, 4:56 du)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review144833
-----------------------------------------------------------
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 342)
<https://reviews.apache.org/r/47837/#comment210972>
Can you elabrote more?
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 350)
<https://reviews.apache.org/r/47837/#comment210970>
Agree that System.gc() is not reliable. That is one the reason I tested the case in a loop for 100 time, and I didn't saw any failure.
Our goal is not to check weak functionality of MapMaker. It's guarantee that weak value will get reclaimed after GC.
This test case is to make sure that we don't mess up with lock entry.
Currently, map has InterProcessReadWriteLock; we want to make sure that when we release, InterProcessReadWriteLock is set to null, not write or read lock.
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java (line 372)
<https://reviews.apache.org/r/47837/#comment210971>
We have been following _ naming for a long time. If you look at other test cases, not test function start with an underscore. But I agree that
chcekLockRelease is a better name.
- Purshotam Shah
On Aug. 3, 2016, 4:56 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2016, 4:56 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/
-----------------------------------------------------------
(Updated Aug. 3, 2016, 4:56 p.m.)
Review request for oozie.
Bugs: OOZIE-2501
https://issues.apache.org/jira/browse/OOZIE-2501
Repository: oozie-git
Description
-------
OOZIE-2501 ZK reentrant lock doesn't work for few cases
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
Diff: https://reviews.apache.org/r/47837/diff/
Testing
-------
Thanks,
Purshotam Shah
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
> On Aug. 1, 2016, 7:55 p.m., Rohini Palaniswamy wrote:
> > core/src/main/java/org/apache/oozie/lock/MemoryLocks.java, lines 90-92
> > <https://reviews.apache.org/r/47837/diff/1-3/?file=1393789#file1393789line90>
> >
> > This is redundant. lockEntry cannot be null here.
ConcurrentMap.putIfAbscent returns the previous value associated with the specified key, or null if there was no mapping for the key.
Refer http://stackoverflow.com/questions/21621803/concurrenthashmap-putifabsent-is-returning-null
> On Aug. 1, 2016, 7:55 p.m., Rohini Palaniswamy wrote:
> > core/src/main/java/org/apache/oozie/service/ZKLocksService.java, lines 151-153
> > <https://reviews.apache.org/r/47837/diff/1-3/?file=1393791#file1393791line151>
> >
> > This is redundant. lockEntry cannot be null here.
ConcurrentMap.putIfAbscent returns the previous value associated with the specified key, or null if there was no mapping for the key.
Refer http://stackoverflow.com/questions/21621803/concurrenthashmap-putifabsent-is-returning-null
- Purshotam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review144379
-----------------------------------------------------------
On Aug. 1, 2016, 6:23 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2016, 6:23 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Rohini Palaniswamy <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review144379
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java (lines 88 - 90)
<https://reviews.apache.org/r/47837/#comment210397>
This is redundant. lockEntry cannot be null here.
core/src/main/java/org/apache/oozie/service/ZKLocksService.java (lines 149 - 151)
<https://reviews.apache.org/r/47837/#comment210398>
This is redundant. lockEntry cannot be null here.
- Rohini Palaniswamy
On Aug. 1, 2016, 6:23 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2016, 6:23 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/
-----------------------------------------------------------
(Updated Aug. 1, 2016, 6:23 p.m.)
Review request for oozie.
Bugs: OOZIE-2501
https://issues.apache.org/jira/browse/OOZIE-2501
Repository: oozie-git
Description
-------
OOZIE-2501 ZK reentrant lock doesn't work for few cases
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
Diff: https://reviews.apache.org/r/47837/diff/
Testing
-------
Thanks,
Purshotam Shah
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/
-----------------------------------------------------------
(Updated July 29, 2016, 9:16 p.m.)
Review request for oozie.
Bugs: OOZIE-2501
https://issues.apache.org/jira/browse/OOZIE-2501
Repository: oozie-git
Description
-------
OOZIE-2501 ZK reentrant lock doesn't work for few cases
Diffs (updated)
-----
core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
Diff: https://reviews.apache.org/r/47837/diff/
Testing
-------
Thanks,
Purshotam Shah
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
> On July 13, 2016, 11:32 p.m., Rohini Palaniswamy wrote:
> > core/src/main/java/org/apache/oozie/service/MemoryLocksService.java, line 33
> > <https://reviews.apache.org/r/47837/diff/1/?file=1393790#file1393790line33>
> >
> > protected
It is used for MemoryLocks which is not subcalss of MemoryLocksService.
- Purshotam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review142151
-----------------------------------------------------------
On May 25, 2016, 6:45 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated May 25, 2016, 6:45 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 2939b604da579adeca399c879425eafab2536b88
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 35fc8a6592b2ba3c5ed4a78b9cc6ed90dd417f40
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 70aa4d7d54c286fec1858a0d0f97f09fff9b4b9f
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Rohini Palaniswamy <ro...@gmail.com>.
> On July 13, 2016, 11:32 p.m., Rohini Palaniswamy wrote:
> > core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java, line 405
> > <https://reviews.apache.org/r/47837/diff/1/?file=1393788#file1393788line405>
> >
> > This code seems to be for a different issue. Can you update with details on why this change is done both in jira and here?
Can you clarify more? This code is not related to this jira. So please add information to the jira on the actual issue that is being fixed here and in the jira or move this to a separate jira. The comment in code mentions what is being fixed, but you need to mention in the jira what is the affected case scenario.
- Rohini
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review142151
-----------------------------------------------------------
On Aug. 1, 2016, 6:23 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2016, 6:23 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Purshotam Shah <pu...@yahoo-inc.com>.
> On July 13, 2016, 11:32 p.m., Rohini Palaniswamy wrote:
> > core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java, line 405
> > <https://reviews.apache.org/r/47837/diff/1/?file=1393788#file1393788line405>
> >
> > This code seems to be for a different issue. Can you update with details on why this change is done both in jira and here?
>
> Rohini Palaniswamy wrote:
> Can you clarify more? This code is not related to this jira. So please add information to the jira on the actual issue that is being fixed here and in the jira or move this to a separate jira. The comment in code mentions what is being fixed, but you need to mention in the jira what is the affected case scenario.
Added comment to JIRA.
- Purshotam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review142151
-----------------------------------------------------------
On Aug. 1, 2016, 6:23 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2016, 6:23 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 41f4430f69cb7a9b132a4000c3e5c8aa7573c0a0
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 952b90d5dfbfeccf4600238f75885c792709ffc7
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java d1acadfff36fff637fb9ccb8e3feffb24248c792
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>
Re: Review Request 47837: OOZIE-2501 ZK reentrant lock doesn't work
for few cases
Posted by Rohini Palaniswamy <ro...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47837/#review142151
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java (line 405)
<https://reviews.apache.org/r/47837/#comment207668>
This code seems to be for a different issue. Can you update with details on why this change is done both in jira and here?
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java (lines 41 - 42)
<https://reviews.apache.org/r/47837/#comment207673>
Add private final back
core/src/main/java/org/apache/oozie/lock/MemoryLocks.java (line 85)
<https://reviews.apache.org/r/47837/#comment207672>
Unnecessary garbage object creation. If get is null, then do putIfAbsent.
core/src/main/java/org/apache/oozie/service/MemoryLocksService.java (line 33)
<https://reviews.apache.org/r/47837/#comment207674>
protected
core/src/main/java/org/apache/oozie/service/ZKLocksService.java (lines 145 - 146)
<https://reviews.apache.org/r/47837/#comment207677>
If get is null, then putIfAbsent
core/src/main/java/org/apache/oozie/service/ZKLocksService.java (lines 169 - 170)
<https://reviews.apache.org/r/47837/#comment207684>
Add private final back
- Rohini Palaniswamy
On May 25, 2016, 6:45 p.m., Purshotam Shah wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47837/
> -----------------------------------------------------------
>
> (Updated May 25, 2016, 6:45 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2501
> https://issues.apache.org/jira/browse/OOZIE-2501
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> OOZIE-2501 ZK reentrant lock doesn't work for few cases
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java 2939b604da579adeca399c879425eafab2536b88
> core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 7d65ac0e24a62086732ec91fc24f89b62469451d
> core/src/main/java/org/apache/oozie/service/MemoryLocksService.java d7c6a89fd47a219b2ec8ea4fe0caf05dc008943b
> core/src/main/java/org/apache/oozie/service/ZKLocksService.java 35fc8a6592b2ba3c5ed4a78b9cc6ed90dd417f40
> core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 61fec19b346748b22df1b58f014c32b1c04c8c1f
> core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 70aa4d7d54c286fec1858a0d0f97f09fff9b4b9f
>
> Diff: https://reviews.apache.org/r/47837/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Purshotam Shah
>
>