[ 
https://issues.apache.org/jira/browse/JCR-2874?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12995289#comment-12995289
 ] 

Thomas Mueller commented on JCR-2874:
-------------------------------------

Hi Angela,

No need to flame. I'm familiar with Jackrabbit, but I just don't like how 
complex it is :-) I know my solution (combine all Jackrabbit projects) doesn't 
please everybody, so let's not do this at the moment. But it's definitely a 
problem if you have to write unit tests for a feature in another project... 
Please tell me if you have a solution for this ;-)

Regards,
Thomas


> Locked helper class improvements
> --------------------------------
>
>                 Key: JCR-2874
>                 URL: https://issues.apache.org/jira/browse/JCR-2874
>             Project: Jackrabbit Content Repository
>          Issue Type: Improvement
>          Components: jackrabbit-jcr-commons, jackrabbit-spi-commons
>            Reporter: Alex Parvulescu
>            Priority: Minor
>         Attachments: lock_upgrade.diff, lock_upgrade_2_tests.patch, 
> lock_upgrade_2_wrapper.patch
>
>
> Currently there are 2 helper classes called Locked, that serve the same 
> purpose:
> one in jcr-commons 
> http://jackrabbit.apache.org/api/2.2/org/apache/jackrabbit/util/Locked.html
> and the other one in spi-commons 
> http://jackrabbit.apache.org/api/2.2/org/apache/jackrabbit/spi/commons/lock/Locked.html
> There was a discussion a while back about deprecating one of them. the issue 
> affected JR 1.4 and it is now closed. I guess the old patches have not been 
> applied: https://issues.apache.org/jira/browse/JCR-1169
> Anyway, I propose to keep the one in jcr-commons and deprecate the other one.
> Also, while we are on the matter, I'd like to propose some improvements to 
> this class:
> 1. make the lock configurable, basically add a flag to say if it is session 
> scoped or not (currently it is hard coded as a session wide lock).
> 2. upgrade the lock code to use the LockManager 
> (http://www.day.com/maven/javax.jcr/javadocs/jcr-2.0/javax/jcr/lock/LockManager.html)
>  - this means that the class will rely only on the JCR 2.0. this is a 
> potentially breaking change, although a lot of the classes in this module 
> depend directly on the 2.0 API
> 3. allow the Locked class to use generics. The biggest issue here is the 
> timeout behavior: on timeout you get the TIMED_OUT token object, and you have 
> to compare the response to that, to make sure that the code ran properly. I 
> think the simplest solution would be to not touch the class directly and 
> build a wrapper class that is generified, and throws a RepositoryException in 
> case of a timeout.
> This would turn this code( from the javadoc)
>   Node counter = ...;
>   Object ret = new Locked() {
>      protected Object run(Node counter) throws RepositoryException {
>          ...
>      }
>   }.with(counter, false);
>   if (ret == Locked.TIMED_OUT) {
>      // do whatever you think is appropriate in this case
>   } else {
>      // get the value
>      long nextValue = ((Long) ret).longValue();
>   }
> into 
>   Node counter = ...;
>   Long ret = new LockedWrapper<Long>() {
>      protected Object run(Node counter) throws RepositoryException {
>          ...
>      }
>   }.with(counter, false);
>   // handle the timeout as a RepositoryException
> 4. lock tests location? (this is more of an observation than an actual issue 
> it came to me via 'find . -name *Lock*Test.java')
> There are some lock tests in jackrabbit-core: 
>  - jackrabbit-core/src/test/java/org/apache/jackrabbit/core/LockTest.java
>  - 
> jackrabbit-core/src/test/java/org/apache/jackrabbit/core/lock/LockTimeoutTest.java
>  - 
> jackrabbit-core/src/test/java/org/apache/jackrabbit/core/lock/ConcurrentLockingWithTransactionsTest.java
>  - 
> jackrabbit-core/src/test/java/org/apache/jackrabbit/core/lock/ExtendedLockingTest.java
>  - 
> jackrabbit-core/src/test/java/org/apache/jackrabbit/core/lock/ConcurrentLockingTest.java
> ...some in jackrabbit-jcr2spi:
>  - 
> jackrabbit-jcr2spi/src/test/java/org/apache/jackrabbit/jcr2spi/lock/OpenScopedLockTest.java
>  - 
> jackrabbit-jcr2spi/src/test/java/org/apache/jackrabbit/jcr2spi/lock/SessionScopedLockTest.java
>  - 
> jackrabbit-jcr2spi/src/test/java/org/apache/jackrabbit/jcr2spi/lock/DeepLockTest.java
>  - 
> jackrabbit-jcr2spi/src/test/java/org/apache/jackrabbit/jcr2spi/lock/AbstractLockTest.java
> and others in jackrabbit-jcr-tests:
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/observation/LockingTest.java
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/lock/SetValueLockExceptionTest.java
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/lock/LockManagerTest.java
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/lock/OpenScopedLockTest.java
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/lock/LockTest.java
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/lock/SessionScopedLockTest.java
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/lock/DeepLockTest.java
>  - 
> jackrabbit-jcr-tests/src/main/java/org/apache/jackrabbit/test/api/lock/AbstractLockTest.java
> I'd like to move this one: org.apache.jackrabbit.core.LockTest from the 
> jackrabbit-core to the jackrabbit-jcr-commons where the implementation class 
> lives.
> I'll add a patch for 1 & 2, but the rest I need your opinion.

-- 
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to