Mailing-List: contact dev-help@jackrabbit.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jackrabbit.apache.org
Date: Tue, 15 Feb 2011 19:00:57 +0000 (UTC)
From: "Alex Parvulescu (JIRA)" <jira@apache.org>
To: dev@jackrabbit.apache.org
Message-ID: 
 <71084474.18365.1297796457463.JavaMail.tomcat@hel.zones.apache.org>
In-Reply-To: 
 <766479919.5222.1296660028976.JavaMail.tomcat@hel.zones.apache.org>
Subject: [jira] Commented: (JCR-2874) Locked helper class improvements
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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

Alex Parvulescu commented on JCR-2874:
--------------------------------------

Thanks Jukka!

Sorry about the tabs, eclipse tricked me into it :)

I'll attach 2 more files, one with the wrapper (proposal number 3), and the other one with the Locked and LockedWrapper tests. 
I took the liberty of updating the existing LockTest class a little bit, I mostly removed the compilation warnings.
I also added a test class for the wrapper, which is heavily inspired by the already existing tests, with some small tweaks.

I'm not sure if we should keep just one test class for both Locked and LockedWrapper.

One other thing, can I file a jira for the junit update? It seems jackrabbit uses 3.8.1 which is really old(Sept 2009 according to this http://sourceforge.net/projects/junit/files/junit/3.8.1/), can we upgrade to a newer version?  4.8.1 maybe?

thanks for your time

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