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

        Alex Parvulescu created issue -
        Alex Parvulescu made changes -
        Field Original Value New Value
        Attachment lock_upgrade.diff [ 12470050 ]
        Alex Parvulescu made changes -
        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 test I need your opinion.
        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.
        Alex Parvulescu made changes -
        Attachment lock_upgrade_2_wrapper.patch [ 12471095 ]
        Alex Parvulescu made changes -
        Attachment lock_upgrade_2_tests.patch [ 12471096 ]

          People

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

            Dates

            • Created:
              Updated:

              Development