Details

    • Bug
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • None
    • 0.99.0, 0.98.5
    • None
    • Reviewed

    Description

      In ProcedureCoordinator and ProcedureMember we synchronize on an instance of ConcurrentMap instead of using the interface methods for dealing with concurrent access.

      Attachments

        1. HBASE-11537.patch
          8 kB
          Michael Stack
        2. HBASE-11537.patch.txt
          8 kB
          Mike Drob
        3. HBASE-11537-0.98.patch
          7 kB
          Andrew Kyle Purtell

        Activity

          mdrob Mike Drob added a comment -

          This patch swaps out the use of synchronization on a ConcurrentMap with calls to putIfAbsent and the conditional remove.

          We also remove a dead store of the result of pool.submit() since we return from the method immediately. And remove the unreachable null check in the catch block because if something threw an exception then it's not possible for the given Future to have been assigned.

          I'm not sure how to write additional tests for this, but the existing tests pass for me. Would love to hear feedback.

          mdrob Mike Drob added a comment - This patch swaps out the use of synchronization on a ConcurrentMap with calls to putIfAbsent and the conditional remove . We also remove a dead store of the result of pool.submit() since we return from the method immediately. And remove the unreachable null check in the catch block because if something threw an exception then it's not possible for the given Future to have been assigned. I'm not sure how to write additional tests for this, but the existing tests pass for me. Would love to hear feedback.
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12656330/HBASE-11537.patch.txt
          against trunk revision .
          ATTACHMENT ID: 12656330

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.master.TestRestartCluster
          org.apache.hadoop.hbase.TestRegionRebalancing

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//console

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12656330/HBASE-11537.patch.txt against trunk revision . ATTACHMENT ID: 12656330 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.master.TestRestartCluster org.apache.hadoop.hbase.TestRegionRebalancing Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10108//console This message is automatically generated.
          stack Michael Stack added a comment -

          Retry against hadoopqa. I don't believe failures related. Checking.

          stack Michael Stack added a comment - Retry against hadoopqa. I don't believe failures related. Checking.
          stack Michael Stack added a comment -

          +1 on patch. LGTM. Looking at the synchronizes, I could not see need to keep procedures changes in line with other changes in state Jonathan Hsieh Do you want to take a looksee? This stuff came in w/ original procedures commit HBASE-7212

          stack Michael Stack added a comment - +1 on patch. LGTM. Looking at the synchronizes, I could not see need to keep procedures changes in line with other changes in state Jonathan Hsieh Do you want to take a looksee? This stuff came in w/ original procedures commit HBASE-7212

          Definitely synchronizing on objects out of the concurrency package is a code smell.

          apurtell Andrew Kyle Purtell added a comment - Definitely synchronizing on objects out of the concurrency package is a code smell.
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12656540/HBASE-11537.patch
          against trunk revision .
          ATTACHMENT ID: 12656540

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 lineLengths. The patch does not introduce lines longer than 100

          +1 site. The mvn site goal succeeds with this patch.

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.master.TestMasterOperationsForRegionReplicas

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//console

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12656540/HBASE-11537.patch against trunk revision . ATTACHMENT ID: 12656540 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 1 warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.master.TestMasterOperationsForRegionReplicas Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10119//console This message is automatically generated.
          jmhsieh Jonathan Hsieh added a comment -

          Nice. lgtm.

          I initially thought changing this in the ProcedureCoordinator introduces a may introduce a potential race condition. I spent a little while trying to find a race, could not, and have been convinced that this new implementation is correct. The only way to insert elements into procedure is the one location where the putIfAbsent is used prevents insertion races and the only removes use the remove if equals ilk which prevents accidentally removing the someone else's insertion race.

          Though a bad smell I think the synchronized based approach is still correct. I'll commit to master and the older branches the patch applies cleanly to.

          jmhsieh Jonathan Hsieh added a comment - Nice. lgtm. I initially thought changing this in the ProcedureCoordinator introduces a may introduce a potential race condition. I spent a little while trying to find a race, could not, and have been convinced that this new implementation is correct. The only way to insert elements into procedure is the one location where the putIfAbsent is used prevents insertion races and the only removes use the remove if equals ilk which prevents accidentally removing the someone else's insertion race. Though a bad smell I think the synchronized based approach is still correct. I'll commit to master and the older branches the patch applies cleanly to.
          jmhsieh Jonathan Hsieh added a comment -

          Thanks for the patch Mike. Committed to master and branch-1. (did not apply cleanly to 98)

          jmhsieh Jonathan Hsieh added a comment - Thanks for the patch Mike. Committed to master and branch-1. (did not apply cleanly to 98)

          Here's what I get when porting to 0.98. Would appreciate a quick look before I commit it.

          apurtell Andrew Kyle Purtell added a comment - Here's what I get when porting to 0.98. Would appreciate a quick look before I commit it.
          mdrob Mike Drob added a comment -

          Jonathan Hsieh - Thanks for looking. I agree that the existing implementation did not have a race condition, but I'm worried that there were already unguarded get/remove calls and a change in behaviour in surrounding code might expose something.

          mdrob Mike Drob added a comment - Jonathan Hsieh - Thanks for looking. I agree that the existing implementation did not have a race condition, but I'm worried that there were already unguarded get/remove calls and a change in behaviour in surrounding code might expose something.
          mdrob Mike Drob added a comment - Andrew Kyle Purtell - LGTM.
          jmhsieh Jonathan Hsieh added a comment -

          andrew. lgtm.

          jmhsieh Jonathan Hsieh added a comment - andrew. lgtm.

          Thanks for looking Mike and Jon. Thanks for the patch Mike. Committed to 0.98

          apurtell Andrew Kyle Purtell added a comment - Thanks for looking Mike and Jon. Thanks for the patch Mike. Committed to 0.98
          hudson Hudson added a comment -

          SUCCESS: Integrated in HBase-TRUNK #5324 (See https://builds.apache.org/job/HBase-TRUNK/5324/)
          HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (jmhsieh: rev 5f4e85d3f98b0456383fa691574198dedcd89bd0)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java
          hudson Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #5324 (See https://builds.apache.org/job/HBase-TRUNK/5324/ ) HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (jmhsieh: rev 5f4e85d3f98b0456383fa691574198dedcd89bd0) hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java
          hudson Hudson added a comment -

          SUCCESS: Integrated in HBase-1.0 #57 (See https://builds.apache.org/job/HBase-1.0/57/)
          HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (jmhsieh: rev 7c020244b5252718d1f1542157264c2a8f2a54aa)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
          hudson Hudson added a comment - SUCCESS: Integrated in HBase-1.0 #57 (See https://builds.apache.org/job/HBase-1.0/57/ ) HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (jmhsieh: rev 7c020244b5252718d1f1542157264c2a8f2a54aa) hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
          hudson Hudson added a comment -

          SUCCESS: Integrated in HBase-0.98 #406 (See https://builds.apache.org/job/HBase-0.98/406/)
          HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (apurtell: rev 4ee4439508aeaf8b2ab4b338b2da7c4f980fc9f3)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
          hudson Hudson added a comment - SUCCESS: Integrated in HBase-0.98 #406 (See https://builds.apache.org/job/HBase-0.98/406/ ) HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (apurtell: rev 4ee4439508aeaf8b2ab4b338b2da7c4f980fc9f3) hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
          hudson Hudson added a comment -

          SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #386 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/386/)
          HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (apurtell: rev 4ee4439508aeaf8b2ab4b338b2da7c4f980fc9f3)

          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java
          • hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
          hudson Hudson added a comment - SUCCESS: Integrated in HBase-0.98-on-Hadoop-1.1 #386 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/386/ ) HBASE-11537 Avoid synchronization on instances of ConcurrentMap (Mike Drob) (apurtell: rev 4ee4439508aeaf8b2ab4b338b2da7c4f980fc9f3) hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureMember.java hbase-server/src/main/java/org/apache/hadoop/hbase/procedure/ProcedureCoordinator.java
          enis Enis Soztutar added a comment -

          Closing this issue after 0.99.0 release.

          enis Enis Soztutar added a comment - Closing this issue after 0.99.0 release.

          People

            mdrob Mike Drob
            mdrob Mike Drob
            Votes:
            0 Vote for this issue
            Watchers:
            Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack

                In order to see discussions, first confirm access to your Slack account(s) in the following workspace(s): ASF