Details

      Description

      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.

      1. lock_upgrade.diff
        8 kB
        Alex Parvulescu
      2. lock_upgrade_2_wrapper.patch
        4 kB
        Alex Parvulescu
      3. lock_upgrade_2_tests.patch
        24 kB
        Alex Parvulescu

        Activity

        Hide
        Alex Parvulescu added a comment -

        Included in this patch:

        1. configurable lock scope
        2. LockManager api

        Show
        Alex Parvulescu added a comment - Included in this patch: 1. configurable lock scope 2. LockManager api
        Hide
        Jukka Zitting added a comment -

        Good stuff, thanks! I committed the patch in revision 1070916.

        3. Agreed, throwing an exception in the timeout case seems like a more natural solution than returning a special sentinel value. Ideally the exception would be a subclass of RepositoryException to make it easier to capture.

        4. We have a bit of a problem with unit testing in jcr-commons, as due to the way dependencies are ordered we can't rely on jackrabbit-core to provide a JCR repository against which to test the jcr-commons functionality. Thus we'd either need to put such jcr-commons integration tests somewhat counterintuitively in jackrabbit-core or set up a complicated mock repository for use within jcr-commons.

        PS. Note that we try to use only spaces for indentation. Your patch had some tabs in it that I cleaned away before committing.

        Show
        Jukka Zitting added a comment - Good stuff, thanks! I committed the patch in revision 1070916. 3. Agreed, throwing an exception in the timeout case seems like a more natural solution than returning a special sentinel value. Ideally the exception would be a subclass of RepositoryException to make it easier to capture. 4. We have a bit of a problem with unit testing in jcr-commons, as due to the way dependencies are ordered we can't rely on jackrabbit-core to provide a JCR repository against which to test the jcr-commons functionality. Thus we'd either need to put such jcr-commons integration tests somewhat counterintuitively in jackrabbit-core or set up a complicated mock repository for use within jcr-commons. PS. Note that we try to use only spaces for indentation. Your patch had some tabs in it that I cleaned away before committing.
        Hide
        Alex Parvulescu added a comment -

        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

        Show
        Alex Parvulescu added a comment - 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
        Hide
        Alex Parvulescu added a comment -

        LockedWrapper class

        Show
        Alex Parvulescu added a comment - LockedWrapper class
        Hide
        Alex Parvulescu added a comment -

        tests

        Show
        Alex Parvulescu added a comment - tests
        Hide
        Thomas Mueller added a comment -

        > We have a bit of a problem with unit testing in jcr-commons, as due to the way dependencies are ordered we can't rely on jackrabbit-core to provide a JCR repository against which to test the jcr-commons functionality. Thus we'd either need to put such jcr-commons integration tests somewhat counterintuitively in jackrabbit-core or set up a complicated mock repository for use within jcr-commons.

        Or we move the jcr-commons code to jackrabbit core (getting rid of jcr-commons).

        I think the disadvantages (having more projects, having the tests of a project in another project) is bigger than the advantage (the theoretical possibility to use jcr-commons with another JCR implementation).

        I'm not saying we should change things now for the current Jackrabbit, but I hope we can simplify things for Jackrabbit 3 (I know I repeat myself).

        Show
        Thomas Mueller added a comment - > We have a bit of a problem with unit testing in jcr-commons, as due to the way dependencies are ordered we can't rely on jackrabbit-core to provide a JCR repository against which to test the jcr-commons functionality. Thus we'd either need to put such jcr-commons integration tests somewhat counterintuitively in jackrabbit-core or set up a complicated mock repository for use within jcr-commons. Or we move the jcr-commons code to jackrabbit core (getting rid of jcr-commons). I think the disadvantages (having more projects, having the tests of a project in another project) is bigger than the advantage (the theoretical possibility to use jcr-commons with another JCR implementation). I'm not saying we should change things now for the current Jackrabbit, but I hope we can simplify things for Jackrabbit 3 (I know I repeat myself).
        Hide
        angela added a comment -

        > Or we move the jcr-commons code to jackrabbit core (getting rid of jcr-commons).

        that's definitely not an option.
        the jcr-commons is used in other modules that by no means should have a dependency to jackrabbit-core.
        sorry, tom, but you keep having a very restricted focus on few details of jackrabbit-core... rather don't mess
        around with things you are not familiar with.

        Show
        angela added a comment - > Or we move the jcr-commons code to jackrabbit core (getting rid of jcr-commons). that's definitely not an option. the jcr-commons is used in other modules that by no means should have a dependency to jackrabbit-core. sorry, tom, but you keep having a very restricted focus on few details of jackrabbit-core... rather don't mess around with things you are not familiar with.
        Hide
        Dominique Pfister added a comment -

        Good to know, Angela, but I don't think it is necessary to get personal.

        Show
        Dominique Pfister added a comment - Good to know, Angela, but I don't think it is necessary to get personal.
        Hide
        Thomas Mueller added a comment -

        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

        Show
        Thomas Mueller added a comment - 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
        Hide
        Randall Hauch added a comment -

        I dislike the idea of moving 'jcr-commons' into 'jackrabbit-core' because I know of people using 'jcr-commons' with ModeShape (another JCR implementation), and they don't have/want dependencies on Jackrabbit.

        BTW, I really wish all these JCR utility classes could go in a separate "JCR Contrib" project that is completely independent of all JCR implementations.

        Show
        Randall Hauch added a comment - I dislike the idea of moving 'jcr-commons' into 'jackrabbit-core' because I know of people using 'jcr-commons' with ModeShape (another JCR implementation), and they don't have/want dependencies on Jackrabbit. BTW, I really wish all these JCR utility classes could go in a separate "JCR Contrib" project that is completely independent of all JCR implementations.
        Hide
        Thomas Mueller added a comment -

        What about adding a dependency of type "test" to jackrabbit-jcr-commons?

        <dependency>
        <groupId>org.apache.jackrabbit</groupId>
        <artifactId>jackrabbit-core</artifactId>
        <version>2.3-SNAPSHOT</version>
        <scope>test</scope>
        </dependency>

        I know it's a circular dependency (jackrabbit-core depends on jackrabbit-jcr-commons). But after adding this dependency on my machine, both projects still build (mvn clean install).

        Show
        Thomas Mueller added a comment - What about adding a dependency of type "test" to jackrabbit-jcr-commons? <dependency> <groupId>org.apache.jackrabbit</groupId> <artifactId>jackrabbit-core</artifactId> <version>2.3-SNAPSHOT</version> <scope>test</scope> </dependency> I know it's a circular dependency (jackrabbit-core depends on jackrabbit-jcr-commons). But after adding this dependency on my machine, both projects still build (mvn clean install).
        Hide
        Jukka Zitting added a comment -

        Re: test dependencies; See JCR-2692 where this was discussed before.

        Show
        Jukka Zitting added a comment - Re: test dependencies; See JCR-2692 where this was discussed before.
        Hide
        Thomas Mueller added a comment -

        OK, when I try to build everything (on trunk), I also get the exception:
        "The projects in the reactor contain a cyclic reference..."

        Show
        Thomas Mueller added a comment - OK, when I try to build everything (on trunk), I also get the exception: "The projects in the reactor contain a cyclic reference..."
        Hide
        Michael Dürig added a comment -

        Unfortunately the cyclic test reference only works as long as you build the single module only. Building the whole project results in above build failure. I experienced this already in JCR-2692...

        Show
        Michael Dürig added a comment - Unfortunately the cyclic test reference only works as long as you build the single module only. Building the whole project results in above build failure. I experienced this already in JCR-2692 ...
        Hide
        Jukka Zitting added a comment -

        I committed the follow-up patches in revision 1077966.

        The LockedWrapper class is a bit artificial construct at the moment, as the "wrapping" part is just about the implementation details. How about if we simply made a cleaned-up version of the Locked class under org.apache.jackrabbit.commons (where we should try to put all new commons code), and deprecate the current version at o.a.j.util?

        Show
        Jukka Zitting added a comment - I committed the follow-up patches in revision 1077966. The LockedWrapper class is a bit artificial construct at the moment, as the "wrapping" part is just about the implementation details. How about if we simply made a cleaned-up version of the Locked class under org.apache.jackrabbit.commons (where we should try to put all new commons code), and deprecate the current version at o.a.j.util?
        Hide
        Alex Parvulescu added a comment -

        I agree about the name.
        The point of this wrapper class was that I could not upgrade the existing one without breaking backwards compatibility. So I decided to create this class that just delegates the method calls, and also ensures type safety.
        To me that is a wrapper
        You can rename it if you find a more suitable name, no worries. Also, feel free to move it, if you feel that there it is better spot for it.

        There already are 2 'Locked' classes: spi-commons (now deprecated) and jcr commons o.a.j.util. Are you sure that a third version of Locked will not create a bit of a confusion?

        Show
        Alex Parvulescu added a comment - I agree about the name. The point of this wrapper class was that I could not upgrade the existing one without breaking backwards compatibility. So I decided to create this class that just delegates the method calls, and also ensures type safety. To me that is a wrapper You can rename it if you find a more suitable name, no worries. Also, feel free to move it, if you feel that there it is better spot for it. There already are 2 'Locked' classes: spi-commons (now deprecated) and jcr commons o.a.j.util. Are you sure that a third version of Locked will not create a bit of a confusion?

          People

          • Assignee:
            Unassigned
            Reporter:
            Alex Parvulescu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development