Re: Review Request 41623: OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in same thread and released

2015-12-29 Thread Rohini Palaniswamy 

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



core/src/main/java/org/apache/oozie/service/ZKLocksService.java (line 200)


The null check should be here instead of isLockHeld.

InterProcessReadWriteLock readWriteLock = zkLocks.get(resource);
//It should never be null, but just in case
if (readWriteLock != null) { 
  if (!isLockHeld(readWriteLock)) {
  
  }
}


- Rohini Palaniswamy


On Dec. 29, 2015, 12:31 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41623/
> ---
> 
> (Updated Dec. 29, 2015, 12:31 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1922
> https://issues.apache.org/jira/browse/OOZIE-1922
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in 
> same thread and released
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 
> ee564b3def2ce42b06f697c3384b1e102ac6a3ba 
>   core/src/main/java/org/apache/oozie/service/MemoryLocksService.java 
> e3eccdb3f95e74198c3d42d19022b4acfc3d18e5 
>   core/src/main/java/org/apache/oozie/service/ZKLocksService.java 
> e3a6bcf04048e140d2ced9d8bdbfe08b01b323e4 
>   core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 
> 0efe31033f63055c08e42202c56c336248271afe 
>   core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 
> 02cc1372dc015713f0bb1f6861b7e4b7455c4f13 
> 
> Diff: https://reviews.apache.org/r/41623/diff/
> 
> 
> Testing
> ---
> 
> TestZKLocksService.testReentrantMultipleCall already has reentrant test cases.
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 41623: OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in same thread and released

2015-12-28 Thread Purshotam Shah 

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

(Updated Dec. 29, 2015, 12:31 a.m.)


Review request for oozie.


Bugs: OOZIE-1922
https://issues.apache.org/jira/browse/OOZIE-1922


Repository: oozie-git


Description
---

OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in 
same thread and released


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 
ee564b3def2ce42b06f697c3384b1e102ac6a3ba 
  core/src/main/java/org/apache/oozie/service/MemoryLocksService.java 
e3eccdb3f95e74198c3d42d19022b4acfc3d18e5 
  core/src/main/java/org/apache/oozie/service/ZKLocksService.java 
e3a6bcf04048e140d2ced9d8bdbfe08b01b323e4 
  core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 
0efe31033f63055c08e42202c56c336248271afe 
  core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 
02cc1372dc015713f0bb1f6861b7e4b7455c4f13 

Diff: https://reviews.apache.org/r/41623/diff/


Testing
---

TestZKLocksService.testReentrantMultipleCall already has reentrant test cases.


Thanks,

Purshotam Shah

Re: Review Request 41623: OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in same thread and released

2015-12-28 Thread Purshotam Shah 


> On Dec. 22, 2015, 11:04 p.m., Rohini Palaniswamy wrote:
> > core/src/main/java/org/apache/oozie/service/ZKLocksService.java, line 201
> > 
> >
> > readWriteLock != null - Instead of adding this condition in multiple 
> > places, either put the whole logic in 
> > 
> > if (readWriteLock != null) {
> > 
> > }
> > 
> > block or make the InterProcessReadWriteLock also part of ZKLockToken 
> > like MemoryLockToken so that you don't have to retrieve from hashmap.

Not sure if i understand this. After removing 
lock.getParticipantNodes().size(), it should be ok.


> On Dec. 22, 2015, 11:04 p.m., Rohini Palaniswamy wrote:
> > core/src/test/java/org/apache/oozie/service/TestZKLocksService.java, line 
> > 508
> > 
> >
> > Test case is most likely flaky. It needs to be changed to make the 
> > other thread acquire the lock after the thread that acquired the lock first 
> > released it. With current logic mostly only one thread will acquire the 
> > lock and release before the assert statement or if the other thread 
> > acquired the lock during sleep(1000), then test will fail. 
> > 
> > Also need a new testcase testReentrantSameThread() - to test reentrant 
> > behaviour in same thread.

>Also need a new testcase testReentrantSameThread() - to test reentrant 
>behaviour in same thread.

There is already a testcase for that, testReentrantMultipleCall.

The behaviour of testcases is that only one thread should be able to acquire 
the lock. Upon release, entry should be removed from hashmap. This was an 
existing testcase, but testcases was incorrect, so I changed it.


- Purshotam


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


On Dec. 22, 2015, 1:11 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41623/
> ---
> 
> (Updated Dec. 22, 2015, 1:11 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1922
> https://issues.apache.org/jira/browse/OOZIE-1922
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in 
> same thread and released
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 
> ee564b3def2ce42b06f697c3384b1e102ac6a3ba 
>   core/src/main/java/org/apache/oozie/service/MemoryLocksService.java 
> e3eccdb3f95e74198c3d42d19022b4acfc3d18e5 
>   core/src/main/java/org/apache/oozie/service/ZKLocksService.java 
> e3a6bcf04048e140d2ced9d8bdbfe08b01b323e4 
>   core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 
> 0efe31033f63055c08e42202c56c336248271afe 
>   core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 
> 02cc1372dc015713f0bb1f6861b7e4b7455c4f13 
> 
> Diff: https://reviews.apache.org/r/41623/diff/
> 
> 
> Testing
> ---
> 
> TestZKLocksService.testReentrantMultipleCall already has reentrant test cases.
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 41623: OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in same thread and released

2015-12-22 Thread Rohini Palaniswamy 

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



core/src/main/java/org/apache/oozie/service/ZKLocksService.java (line 201)


readWriteLock != null - Instead of adding this condition in multiple 
places, either put the whole logic in 

if (readWriteLock != null) {

}

block or make the InterProcessReadWriteLock also part of ZKLockToken like 
MemoryLockToken so that you don't have to retrieve from hashmap.



core/src/test/java/org/apache/oozie/service/TestZKLocksService.java (line 484)


Test case is most likely flaky. It needs to be changed to make the other 
thread acquire the lock after the thread that acquired the lock first released 
it. With current logic mostly only one thread will acquire the lock and release 
before the assert statement or if the other thread acquired the lock during 
sleep(1000), then test will fail. 

Also need a new testcase testReentrantSameThread() - to test reentrant 
behaviour in same thread.


- Rohini Palaniswamy


On Dec. 22, 2015, 1:11 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41623/
> ---
> 
> (Updated Dec. 22, 2015, 1:11 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1922
> https://issues.apache.org/jira/browse/OOZIE-1922
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in 
> same thread and released
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 
> ee564b3def2ce42b06f697c3384b1e102ac6a3ba 
>   core/src/main/java/org/apache/oozie/service/MemoryLocksService.java 
> e3eccdb3f95e74198c3d42d19022b4acfc3d18e5 
>   core/src/main/java/org/apache/oozie/service/ZKLocksService.java 
> e3a6bcf04048e140d2ced9d8bdbfe08b01b323e4 
>   core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 
> 0efe31033f63055c08e42202c56c336248271afe 
>   core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 
> 02cc1372dc015713f0bb1f6861b7e4b7455c4f13 
> 
> Diff: https://reviews.apache.org/r/41623/diff/
> 
> 
> Testing
> ---
> 
> TestZKLocksService.testReentrantMultipleCall already has reentrant test cases.
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 41623: OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in same thread and released

2015-12-22 Thread Robert Kanter 

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

Ship it!



core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 


Don't we still need this line so it can connect to ZooKeeper etc?


- Robert Kanter


On Dec. 22, 2015, 1:11 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41623/
> ---
> 
> (Updated Dec. 22, 2015, 1:11 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1922
> https://issues.apache.org/jira/browse/OOZIE-1922
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in 
> same thread and released
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 
> ee564b3def2ce42b06f697c3384b1e102ac6a3ba 
>   core/src/main/java/org/apache/oozie/service/MemoryLocksService.java 
> e3eccdb3f95e74198c3d42d19022b4acfc3d18e5 
>   core/src/main/java/org/apache/oozie/service/ZKLocksService.java 
> e3a6bcf04048e140d2ced9d8bdbfe08b01b323e4 
>   core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 
> 0efe31033f63055c08e42202c56c336248271afe 
>   core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 
> 02cc1372dc015713f0bb1f6861b7e4b7455c4f13 
> 
> Diff: https://reviews.apache.org/r/41623/diff/
> 
> 
> Testing
> ---
> 
> TestZKLocksService.testReentrantMultipleCall already has reentrant test cases.
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Review Request 41623: OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in same thread and released

2015-12-21 Thread Purshotam Shah 

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

Review request for oozie.


Bugs: OOZIE-1922
https://issues.apache.org/jira/browse/OOZIE-1922


Repository: oozie-git


Description
---

OOZIE-1922 - MemoryLocksService fails if lock is acquired multiple times in 
same thread and released


Diffs
-

  core/src/main/java/org/apache/oozie/lock/MemoryLocks.java 
ee564b3def2ce42b06f697c3384b1e102ac6a3ba 
  core/src/main/java/org/apache/oozie/service/MemoryLocksService.java 
e3eccdb3f95e74198c3d42d19022b4acfc3d18e5 
  core/src/main/java/org/apache/oozie/service/ZKLocksService.java 
e3a6bcf04048e140d2ced9d8bdbfe08b01b323e4 
  core/src/test/java/org/apache/oozie/lock/TestMemoryLocks.java 
0efe31033f63055c08e42202c56c336248271afe 
  core/src/test/java/org/apache/oozie/service/TestZKLocksService.java 
02cc1372dc015713f0bb1f6861b7e4b7455c4f13 

Diff: https://reviews.apache.org/r/41623/diff/


Testing
---

TestZKLocksService.testReentrantMultipleCall already has reentrant test cases.


Thanks,

Purshotam Shah