Description
In ProcedureCoordinator and ProcedureMember we synchronize on an instance of ConcurrentMap instead of using the interface methods for dealing with concurrent access.
Attachments
Attachments
- HBASE-11537.patch
- 8 kB
- Michael Stack
- HBASE-11537.patch.txt
- 8 kB
- Mike Drob
- HBASE-11537-0.98.patch
- 7 kB
- Andrew Kyle Purtell
Activity
-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.
+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.
-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.
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.
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.
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.
Thanks for looking Mike and Jon. Thanks for the patch Mike. Committed to 0.98
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
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
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
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
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.