Uploaded image for project: 'Oozie'
  1. Oozie
  2. OOZIE-1922

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.3.0
    • Component/s: None
    • Labels:
      None

      Description

      ReentrantLock can be acquired multiple times in same thread. For multiple acquire call, ReentrantLock hold count is incremented by one.
      So if we acquire lock multiple time from same thread, all will be successful and hold count is increased for every call.
      But if we release lock, MemoryLocksService ignore the count and deletes the lock. Even if it's held by some command.

      Simple step can reproduce it.

              service.getWriteLock("test", 5000); //writeHoldCount = 1
              MemoryLockToken lock = (MemoryLockToken)service.getWriteLock("test", 5000); //writeHoldCount = 2
              lock.release(); //writeHoldCount = 1
              lock = (MemoryLockToken)service.getWriteLock("test", 5000); //writeHoldCount = 1, it should be 2.
      
              @Override
              public void release() {
                  int val = rwLock.getQueueLength();
                  if (val == 0) {
                      synchronized (locks) {
                          locks.remove(resource);
                      }
                  }
                  lock.unlock();
              }
          }
      

      MemoryLocks should check hold count before removing lock.

      1. OOZIE-1922-V3.patch
        12 kB
        Purshotam Shah
      2. OOZIE-1922-V2.patch
        12 kB
        Purshotam Shah
      3. OOZIE-1922-V1.patch
        13 kB
        Purshotam Shah
      4. OOZIE-1922.3.patch
        4 kB
        Azrael Seoeun
      5. OOZIE-1922.2.patch
        4 kB
        Azrael Seoeun
      6. OOZIE-1922.1.patch
        5 kB
        Azrael Seoeun

        Issue Links

          Activity

          Hide
          seoeun25 Azrael Seoeun added a comment -

          Purshotam Shah can you review please?

          Show
          seoeun25 Azrael Seoeun added a comment - Purshotam Shah can you review please?
          Hide
          seoeun25 Azrael Seoeun added a comment -

          Sorry, I just see the OOZIE-1923 and TODO comment part.

          Show
          seoeun25 Azrael Seoeun added a comment - Sorry, I just see the OOZIE-1923 and TODO comment part.
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 1 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS
          . Tests run: 1507
          . Tests failed: 1
          . Tests errors: 0

          . The patch failed the following testcases:

          . testResetSequence_withMultiThread(org.apache.oozie.service.TestZKUUIDService)

          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/1436/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1507 . Tests failed: 1 . Tests errors: 0 . The patch failed the following testcases: . testResetSequence_withMultiThread(org.apache.oozie.service.TestZKUUIDService) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/1436/
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 1 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS - patch does not compile, cannot run testcases
          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/1616/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS - patch does not compile, cannot run testcases +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/1616/
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 1 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS
          . Tests run: 1510
          . Tests failed: 7
          . Tests errors: 2

          . The patch failed the following testcases:

          . testBundleId(org.apache.oozie.servlet.TestBulkMonitorWebServiceAPI)
          . testMemoryUsageAndSpeed(org.apache.oozie.service.TestPartitionDependencyManagerService)
          . testPigScript(org.apache.oozie.action.hadoop.TestPigMainWithOldAPI)
          . testPigScript(org.apache.oozie.action.hadoop.TestPigMain)
          . testEmbeddedPigWithinPython(org.apache.oozie.action.hadoop.TestPigMain)
          . testPig_withNullExternalID(org.apache.oozie.action.hadoop.TestPigMain)
          . testMain(org.apache.oozie.action.hadoop.TestHiveMain)

          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/1726/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1510 . Tests failed: 7 . Tests errors: 2 . The patch failed the following testcases: . testBundleId(org.apache.oozie.servlet.TestBulkMonitorWebServiceAPI) . testMemoryUsageAndSpeed(org.apache.oozie.service.TestPartitionDependencyManagerService) . testPigScript(org.apache.oozie.action.hadoop.TestPigMainWithOldAPI) . testPigScript(org.apache.oozie.action.hadoop.TestPigMain) . testEmbeddedPigWithinPython(org.apache.oozie.action.hadoop.TestPigMain) . testPig_withNullExternalID(org.apache.oozie.action.hadoop.TestPigMain) . testMain(org.apache.oozie.action.hadoop.TestHiveMain) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/1726/
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 1 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          -1 COMPILE
          . -1 HEAD does not compile
          . -1 patch does not compile
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS - patch does not compile, cannot run testcases
          -1 DISTRO
          . -1 distro tarball fails with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/1849/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings -1 COMPILE . -1 HEAD does not compile . -1 patch does not compile . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS - patch does not compile, cannot run testcases -1 DISTRO . -1 distro tarball fails with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/1849/
          Hide
          jaydeepvishwakarma Jaydeep Vishwakarma added a comment -

          Azrael Seoeun, OOZIE-1923 has been closed. Are you planning to update the patch?

          Show
          jaydeepvishwakarma Jaydeep Vishwakarma added a comment - Azrael Seoeun , OOZIE-1923 has been closed. Are you planning to update the patch?
          Hide
          seoeun25 Azrael Seoeun added a comment -

          Upload new patch rebased on the latest branch.

          Show
          seoeun25 Azrael Seoeun added a comment - Upload new patch rebased on the latest branch.
          Hide
          seoeun25 Azrael Seoeun added a comment -

          OOZIE-1922.2.patch included wrong file. I upload the fixed one.

          Show
          seoeun25 Azrael Seoeun added a comment - OOZIE-1922 .2.patch included wrong file. I upload the fixed one.
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 1 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS
          . Tests run: 1704
          . Tests failed: 8
          . Tests errors: 2

          . The patch failed the following testcases:

          . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration)
          . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation)
          . testUpdateSLA(org.apache.oozie.sla.TestSLAService)
          . testMain(org.apache.oozie.action.hadoop.TestHiveMain)
          . testPigScript(org.apache.oozie.action.hadoop.TestPigMain)
          . testPig_withNullExternalID(org.apache.oozie.action.hadoop.TestPigMain)
          . testEmbeddedPigWithinPython(org.apache.oozie.action.hadoop.TestPigMain)
          . testPigScript(org.apache.oozie.action.hadoop.TestPigMainWithOldAPI)

          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/2652/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 1 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1704 . Tests failed: 8 . Tests errors: 2 . The patch failed the following testcases: . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration) . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation) . testUpdateSLA(org.apache.oozie.sla.TestSLAService) . testMain(org.apache.oozie.action.hadoop.TestHiveMain) . testPigScript(org.apache.oozie.action.hadoop.TestPigMain) . testPig_withNullExternalID(org.apache.oozie.action.hadoop.TestPigMain) . testEmbeddedPigWithinPython(org.apache.oozie.action.hadoop.TestPigMain) . testPigScript(org.apache.oozie.action.hadoop.TestPigMainWithOldAPI) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/2652/
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 2 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS
          . Tests run: 1706
          . Tests failed: 3
          . Tests errors: 0

          . The patch failed the following testcases:

          . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration)
          . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation)
          . testUpdateSLA(org.apache.oozie.sla.TestSLAService)

          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/2686/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 2 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1706 . Tests failed: 3 . Tests errors: 0 . The patch failed the following testcases: . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration) . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation) . testUpdateSLA(org.apache.oozie.sla.TestSLAService) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/2686/
          Hide
          rohini Rohini Palaniswamy added a comment -

          See couple of issues with the patch.
          1) rwLock.getQueueLength() == 0 && rwLock.getWriteHoldCount() == 0 && rwLock.getReadHoldCount() == 0
          This only checks for if there are no waiting threads and if all locks held by this thread are released before removing the lock. It does not take into account if any other thread is holding read or write lock. You will also have to make use of the following APIs in the logic.
          https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html#getReadLockCount()
          https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html#isWriteLocked()
          https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html#isWriteLockedByCurrentThread()

          2)

          & !readWriteLock.readLock().isAcquiredInThisProcess()
          +                                && !readWriteLock.writeLock().isAcquiredInThisProcess()
          
          • Should be && instead of &.
          • isAcquiredInThisProcess stands for if acquired in this JVM. You are trying to remove it from current node's zkLocks HashMap if the node does not hold lock anymore. That is good, but lock.getParticipantNodes().size() == 0 condition will prevent it if another node is hold the lock. So you will have to remove that condition.

          Haven't looked at the test cases yet. Can you put the revised patch in review board as it is hard to read logic for the test cases.

          3) Related to OOZIE-2394 patch:
          Even though acquire() may not cause a ZK call for a reentrant lock, looking at the isAcquiredInThisProcess()/getParticipantNodes() APIs used in release() call, I am concerned that those will be new calls to ZK and might cause performance degradation if synchronous is removed in OOZIE-2394. Can you run one test case enabling debug logging for ZK and see how many calls are made with and without removal of synchronous. If there is increase by even one ZK call, I would like to retain the synchronous method. Cleanup of removing very few lines of simple code is not worth for the cost of a ZK call.

          Show
          rohini Rohini Palaniswamy added a comment - See couple of issues with the patch. 1) rwLock.getQueueLength() == 0 && rwLock.getWriteHoldCount() == 0 && rwLock.getReadHoldCount() == 0 This only checks for if there are no waiting threads and if all locks held by this thread are released before removing the lock. It does not take into account if any other thread is holding read or write lock. You will also have to make use of the following APIs in the logic. https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html#getReadLockCount( ) https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html#isWriteLocked( ) https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html#isWriteLockedByCurrentThread( ) 2) & !readWriteLock.readLock().isAcquiredInThisProcess() + && !readWriteLock.writeLock().isAcquiredInThisProcess() Should be && instead of &. isAcquiredInThisProcess stands for if acquired in this JVM. You are trying to remove it from current node's zkLocks HashMap if the node does not hold lock anymore. That is good, but lock.getParticipantNodes().size() == 0 condition will prevent it if another node is hold the lock. So you will have to remove that condition. Haven't looked at the test cases yet. Can you put the revised patch in review board as it is hard to read logic for the test cases. 3) Related to OOZIE-2394 patch: Even though acquire() may not cause a ZK call for a reentrant lock, looking at the isAcquiredInThisProcess()/getParticipantNodes() APIs used in release() call, I am concerned that those will be new calls to ZK and might cause performance degradation if synchronous is removed in OOZIE-2394 . Can you run one test case enabling debug logging for ZK and see how many calls are made with and without removal of synchronous. If there is increase by even one ZK call, I would like to retain the synchronous method. Cleanup of removing very few lines of simple code is not worth for the cost of a ZK call.
          Hide
          rohini Rohini Palaniswamy added a comment -

          Can you put the revised patch in review board as it is hard to read logic for the test cases.

          Ignore this. See that you have already uploaded the patch to review board.

          Show
          rohini Rohini Palaniswamy added a comment - Can you put the revised patch in review board as it is hard to read logic for the test cases. Ignore this. See that you have already uploaded the patch to review board.
          Hide
          rohini Rohini Palaniswamy added a comment -

          Couple of additional comments in review board.

          Show
          rohini Rohini Palaniswamy added a comment - Couple of additional comments in review board.
          Hide
          puru Purshotam Shah added a comment - - edited

          This only checks for if there are no waiting threads and if all locks held by this thread are released before removing the lock. It does not take into account if any other thread is holding read or write lock. You will also have to make use of the following APIs in the logic.

          Sine we are in release function, which means that lock is acquired by current thread. So, we don't have to check if lock is held by other thread or not.

          Even though acquire() may not cause a ZK call for a reentrant lock, looking at the isAcquiredInThisProcess()/getParticipantNodes() APIs used in release() call, I am concerned that those will be new calls to ZK and might cause performance degradation if synchronous is removed in OOZIE-2394

          isAcquiredInThisProcess() doesn't make any ZK call.

          InterProcessMutex.java
          @Override
              public boolean isAcquiredInThisProcess()
              {
                  return (threadData.size() > 0);
              }
          

          getParticipantNodes() does. We may not need getParticipantNodes, since lock is acquired by current host, it won't be to acquire by other hosts.

          !readWriteLock.readLock().isAcquiredInThisProcess() && !readWriteLock.writeLock().isAcquiredInThisProcess() is enough.

          I didn't remove getParticipantNodes because it was already part of the condition and was not sure if it's break anything.
          If you are really concern about performance, we can remove getParticipantNodes from condition.

          Show
          puru Purshotam Shah added a comment - - edited This only checks for if there are no waiting threads and if all locks held by this thread are released before removing the lock. It does not take into account if any other thread is holding read or write lock. You will also have to make use of the following APIs in the logic. Sine we are in release function, which means that lock is acquired by current thread. So, we don't have to check if lock is held by other thread or not. Even though acquire() may not cause a ZK call for a reentrant lock, looking at the isAcquiredInThisProcess()/getParticipantNodes() APIs used in release() call, I am concerned that those will be new calls to ZK and might cause performance degradation if synchronous is removed in OOZIE-2394 isAcquiredInThisProcess() doesn't make any ZK call. InterProcessMutex.java @Override public boolean isAcquiredInThisProcess() { return (threadData.size() > 0); } getParticipantNodes() does. We may not need getParticipantNodes, since lock is acquired by current host, it won't be to acquire by other hosts. !readWriteLock.readLock().isAcquiredInThisProcess() && !readWriteLock.writeLock().isAcquiredInThisProcess() is enough. I didn't remove getParticipantNodes because it was already part of the condition and was not sure if it's break anything. If you are really concern about performance, we can remove getParticipantNodes from condition.
          Hide
          rohini Rohini Palaniswamy added a comment -

          Sine we are in release function, which means that lock is acquired by current thread. So, we don't have to check if lock is held by other thread or not.

          You have to. If it is read locks, more than one thread can hold the lock. Currently only write locks are acquired in the code though, but still logic needs to be correct as there are APIs to acquire read lock.

          I didn't remove getParticipantNodes because it was already part of the condition and was not sure if it's break anything.

          If you are really concern about performance, we can remove getParticipantNodes from condition.
          Yes. Performance and load on zookeeper is very important.

          Show
          rohini Rohini Palaniswamy added a comment - Sine we are in release function, which means that lock is acquired by current thread. So, we don't have to check if lock is held by other thread or not. You have to. If it is read locks, more than one thread can hold the lock. Currently only write locks are acquired in the code though, but still logic needs to be correct as there are APIs to acquire read lock. I didn't remove getParticipantNodes because it was already part of the condition and was not sure if it's break anything. If you are really concern about performance, we can remove getParticipantNodes from condition. Yes. Performance and load on zookeeper is very important.
          Hide
          puru Purshotam Shah added a comment -

          You have to. If it is read locks, more than one thread can hold the lock. Currently only write locks are acquired in the code though, but still logic needs to be correct as there are APIs to acquire read lock.

          I don't think we will ever support readlocks. We don't have to convolute our logic. I will put TODO in code for readlock support.

          Show
          puru Purshotam Shah added a comment - You have to. If it is read locks, more than one thread can hold the lock. Currently only write locks are acquired in the code though, but still logic needs to be correct as there are APIs to acquire read lock. I don't think we will ever support readlocks. We don't have to convolute our logic. I will put TODO in code for readlock support.
          Hide
          rohini Rohini Palaniswamy added a comment -

          I don't think we will ever support readlocks. We don't have to convolute our logic. I will put TODO in code for readlock support.

          I would prefer that we rather use the correct logic instead instead of TODO as it is simple to fix. It is just couple more conditions which do not cost much. Don't like the idea of breaking it considering that the rest of code for read locks is good even though not used.

          Show
          rohini Rohini Palaniswamy added a comment - I don't think we will ever support readlocks. We don't have to convolute our logic. I will put TODO in code for readlock support. I would prefer that we rather use the correct logic instead instead of TODO as it is simple to fix. It is just couple more conditions which do not cost much. Don't like the idea of breaking it considering that the rest of code for read locks is good even though not used.
          Hide
          puru Purshotam Shah added a comment -

          It's not that simple, mainly for ZKlock. We need to use getParticipantNodes() in logic. This will call ZK every time, why to make unnecessary calls if we are not going to use it.

          Show
          puru Purshotam Shah added a comment - It's not that simple, mainly for ZKlock. We need to use getParticipantNodes() in logic. This will call ZK every time, why to make unnecessary calls if we are not going to use it.
          Hide
          rohini Rohini Palaniswamy added a comment -

          It's not that simple, mainly for ZKlock. We need to use getParticipantNodes() in logic

          Why do you need getParticipantNodes() in logic. You just need to remove from local hashmap when no thread in that jvm holds the lock anymore. It should not matter if another node has a lock.

          Show
          rohini Rohini Palaniswamy added a comment - It's not that simple, mainly for ZKlock. We need to use getParticipantNodes() in logic Why do you need getParticipantNodes() in logic. You just need to remove from local hashmap when no thread in that jvm holds the lock anymore. It should not matter if another node has a lock.
          Hide
          puru Purshotam Shah added a comment -

          Yes, you are correct we don't need to use getParticipantNodes().

          Show
          puru Purshotam Shah added a comment - Yes, you are correct we don't need to use getParticipantNodes().
          Hide
          rohini Rohini Palaniswamy added a comment -

          Thanks. New logic for memory looks solid now.

          +            InterProcessReadWriteLock readWriteLock = zkLocks.get(resource);
          // This shouldn't happen, but if it does, then we should remove entry from map
          +            if (readWriteLock == null) {
          +                return false;
          +            }
          

          Above code means that it is not in the hashmap already, so trying to remove from hashmap does not make sense. if (readWriteLock == null) check should be before and outside of isLockHeld.

          Show
          rohini Rohini Palaniswamy added a comment - Thanks. New logic for memory looks solid now. + InterProcessReadWriteLock readWriteLock = zkLocks.get(resource); // This shouldn't happen, but if it does, then we should remove entry from map + if (readWriteLock == null ) { + return false ; + } Above code means that it is not in the hashmap already, so trying to remove from hashmap does not make sense. if (readWriteLock == null) check should be before and outside of isLockHeld.
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 2 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS
          . Tests run: 1706
          . Tests failed: 7
          . Tests errors: 0

          . The patch failed the following testcases:

          . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration)
          . testPurgeXCommandFailed(org.apache.oozie.command.TestPurgeXCommand)
          . testPurgeWFWithSubWF1(org.apache.oozie.command.TestPurgeXCommand)
          . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation)
          . testCoord_throwException(org.apache.oozie.command.coord.TestCoordChangeXCommand)
          . testRecovery(org.apache.oozie.action.hadoop.TestJavaActionExecutor)
          . testUpdateSLA(org.apache.oozie.sla.TestSLAService)

          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/2689/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 2 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1706 . Tests failed: 7 . Tests errors: 0 . The patch failed the following testcases: . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration) . testPurgeXCommandFailed(org.apache.oozie.command.TestPurgeXCommand) . testPurgeWFWithSubWF1(org.apache.oozie.command.TestPurgeXCommand) . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation) . testCoord_throwException(org.apache.oozie.command.coord.TestCoordChangeXCommand) . testRecovery(org.apache.oozie.action.hadoop.TestJavaActionExecutor) . testUpdateSLA(org.apache.oozie.sla.TestSLAService) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/2689/
          Hide
          puru Purshotam Shah added a comment -

          I fix that. I have also optimized the memory lock condition.
          getReadLockCount is enough, it will also give the count of readlock held by current thread. We don't have to use getReadHoldCount.

          Show
          puru Purshotam Shah added a comment - I fix that. I have also optimized the memory lock condition. getReadLockCount is enough, it will also give the count of readlock held by current thread. We don't have to use getReadHoldCount.
          Hide
          rohini Rohini Palaniswamy added a comment -

          +1

          Show
          rohini Rohini Palaniswamy added a comment - +1
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 2 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS - patch does not compile, cannot run testcases
          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/2690/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 2 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS - patch does not compile, cannot run testcases +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/2690/
          Hide
          hadoopqa Hadoop QA added a comment -

          Testing JIRA OOZIE-1922

          Cleaning local git workspace

          ----------------------------

          +1 PATCH_APPLIES
          +1 CLEAN
          +1 RAW_PATCH_ANALYSIS
          . +1 the patch does not introduce any @author tags
          . +1 the patch does not introduce any tabs
          . +1 the patch does not introduce any trailing spaces
          . +1 the patch does not introduce any line longer than 132
          . +1 the patch does adds/modifies 2 testcase(s)
          +1 RAT
          . +1 the patch does not seem to introduce new RAT warnings
          +1 JAVADOC
          . +1 the patch does not seem to introduce new Javadoc warnings
          +1 COMPILE
          . +1 HEAD compiles
          . +1 patch compiles
          . +1 the patch does not seem to introduce new javac warnings
          +1 BACKWARDS_COMPATIBILITY
          . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations
          . +1 the patch does not modify JPA files
          -1 TESTS
          . Tests run: 1706
          . Tests failed: 6
          . Tests errors: 0

          . The patch failed the following testcases:

          . testPurgeXCommandFailed(org.apache.oozie.command.TestPurgeXCommand)
          . testPurgeWFWithSubWF1(org.apache.oozie.command.TestPurgeXCommand)
          . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration)
          . testMessage_withMixedStatus(org.apache.oozie.command.coord.TestAbandonedCoordChecker)
          . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation)
          . testPauseBundleAndCoordinator(org.apache.oozie.service.TestPauseTransitService)

          +1 DISTRO
          . +1 distro tarball builds with the patch

          ----------------------------
          -1 Overall result, please check the reported -1(s)

          The full output of the test-patch run is available at

          . https://builds.apache.org/job/oozie-trunk-precommit-build/2691/

          Show
          hadoopqa Hadoop QA added a comment - Testing JIRA OOZIE-1922 Cleaning local git workspace ---------------------------- +1 PATCH_APPLIES +1 CLEAN +1 RAW_PATCH_ANALYSIS . +1 the patch does not introduce any @author tags . +1 the patch does not introduce any tabs . +1 the patch does not introduce any trailing spaces . +1 the patch does not introduce any line longer than 132 . +1 the patch does adds/modifies 2 testcase(s) +1 RAT . +1 the patch does not seem to introduce new RAT warnings +1 JAVADOC . +1 the patch does not seem to introduce new Javadoc warnings +1 COMPILE . +1 HEAD compiles . +1 patch compiles . +1 the patch does not seem to introduce new javac warnings +1 BACKWARDS_COMPATIBILITY . +1 the patch does not change any JPA Entity/Colum/Basic/Lob/Transient annotations . +1 the patch does not modify JPA files -1 TESTS . Tests run: 1706 . Tests failed: 6 . Tests errors: 0 . The patch failed the following testcases: . testPurgeXCommandFailed(org.apache.oozie.command.TestPurgeXCommand) . testPurgeWFWithSubWF1(org.apache.oozie.command.TestPurgeXCommand) . testForNoDuplicates(org.apache.oozie.event.TestEventGeneration) . testMessage_withMixedStatus(org.apache.oozie.command.coord.TestAbandonedCoordChecker) . testSamplers(org.apache.oozie.util.TestMetricsInstrumentation) . testPauseBundleAndCoordinator(org.apache.oozie.service.TestPauseTransitService) +1 DISTRO . +1 distro tarball builds with the patch ---------------------------- -1 Overall result, please check the reported -1(s) The full output of the test-patch run is available at . https://builds.apache.org/job/oozie-trunk-precommit-build/2691/
          Hide
          puru Purshotam Shah added a comment -

          Thanks Rohini for review. Committed to trunk.

          Show
          puru Purshotam Shah added a comment - Thanks Rohini for review. Committed to trunk.
          Hide
          rkanter Robert Kanter added a comment -

          Closing issue; Oozie 4.3.0 is released.

          Show
          rkanter Robert Kanter added a comment - Closing issue; Oozie 4.3.0 is released.

            People

            • Assignee:
              puru Purshotam Shah
              Reporter:
              puru Purshotam Shah
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development