Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-11685

Incr/decr on the reference count of HConnectionImplementation need be atomic

Details

    • Bug
    • Status: Closed
    • Minor
    • Resolution: Abandoned
    • None
    • None
    • Client
    • None

    Description

      Currently, the incr/decr operation on the ref count of HConnectionImplementation are not atomic. This may cause that the ref count always be larger than 0 and the connection never be closed.

          /**
           * Increment this client's reference count.
           */
          void incCount() {
            ++refCount;
          }
      
          /**
           * Decrement this client's reference count.
           */
          void decCount() {
            if (refCount > 0) {
              --refCount;
            }
          }
      

      Attachments

        1. HBASE-11685-trunk-v6.diff
          2 kB
          Shaohui Liu
        2. HBASE-11685-trunk-v5.diff
          2 kB
          Shaohui Liu
        3. HBASE-11685-trunk-v4.diff
          2 kB
          Shaohui Liu
        4. HBASE-11685-trunk-v3.diff
          2 kB
          Shaohui Liu
        5. HBASE-11685-trunk-v2.diff
          2 kB
          Shaohui Liu
        6. HBASE-11685-trunk-v1.diff
          2 kB
          Shaohui Liu

        Activity

          liushaohui Shaohui Liu added a comment -

          Change the refCount from int to AtomicInteger

          liushaohui Shaohui Liu added a comment - Change the refCount from int to AtomicInteger
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12660120/HBASE-11685-trunk-v1.diff
          against trunk revision .
          ATTACHMENT ID: 12660120

          +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.client.TestClientNoCluster

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//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/12660120/HBASE-11685-trunk-v1.diff against trunk revision . ATTACHMENT ID: 12660120 +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.client.TestClientNoCluster Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10313//console This message is automatically generated.
          liushaohui Shaohui Liu added a comment -

          Fix failed test: TestClientNoCluster

          liushaohui Shaohui Liu added a comment - Fix failed test: TestClientNoCluster

          This change does not preserve the old behavior where we avoid taking refCount negative. A concern? We should warn loudly if this goes negative at least I think.

               /**
                * Decrement this client's reference count.
                */
               void decCount() {
          -      if (refCount > 0) {
          -        --refCount;
          -      }
          +      refCount.decrementAndGet();
               }
          
          apurtell Andrew Kyle Purtell added a comment - This change does not preserve the old behavior where we avoid taking refCount negative. A concern? We should warn loudly if this goes negative at least I think. /** * Decrement this client's reference count. */ void decCount() { - if (refCount > 0) { - --refCount; - } + refCount.decrementAndGet(); }
          liushaohui Shaohui Liu added a comment -

          Add warn log when the ref count is nagative.
          Thanks for apurtell's review.

          liushaohui Shaohui Liu added a comment - Add warn log when the ref count is nagative. Thanks for apurtell 's review.
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12660302/HBASE-11685-trunk-v3.diff
          against trunk revision .
          ATTACHMENT ID: 12660302

          +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.util.TestHBaseFsck
          org.apache.hadoop.hbase.master.TestMasterOperationsForRegionReplicas

          -1 core zombie tests. There are 3 zombie test(s):

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//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/12660302/HBASE-11685-trunk-v3.diff against trunk revision . ATTACHMENT ID: 12660302 +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.util.TestHBaseFsck org.apache.hadoop.hbase.master.TestMasterOperationsForRegionReplicas -1 core zombie tests . There are 3 zombie test(s): Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10332//console This message is automatically generated.

          There's a spelling error in the log message. Please change "nagative" to "negative". Is there any way to identify for which connection the ref count went negative? Would aid in debugging. Otherwise lgtm.

          apurtell Andrew Kyle Purtell added a comment - There's a spelling error in the log message. Please change "nagative" to "negative". Is there any way to identify for which connection the ref count went negative? Would aid in debugging. Otherwise lgtm.
          liushaohui Shaohui Liu added a comment -

          Fix the typo and add connection name in warn log for debug.
          Thanks for apurtell's suggestion.

          liushaohui Shaohui Liu added a comment - Fix the typo and add connection name in warn log for debug. Thanks for apurtell 's suggestion.

          Sorry to keep coming back to this.

                */
               boolean isZeroReference() {
          -      return refCount == 0;
          +      return refCount.get() == 0;
               }
          

          We use isZeroReference to detect if a connection can be reaped. If the count goes negative, and it can now after these changes, then isZeroReference will return false and the connection will never be closed out. I really think we need to preserve the earlier logic. This change seems likely to introduce a new class of bug

          apurtell Andrew Kyle Purtell added a comment - Sorry to keep coming back to this. */ boolean isZeroReference() { - return refCount == 0; + return refCount.get() == 0; } We use isZeroReference to detect if a connection can be reaped. If the count goes negative, and it can now after these changes, then isZeroReference will return false and the connection will never be closed out. I really think we need to preserve the earlier logic. This change seems likely to introduce a new class of bug
          liushaohui Shaohui Liu added a comment -

          apurtell

          I really think we need to preserve the earlier logic

          I'm sorry that i didn't get your meanings? is the earlier logic just the current codebase?

          The current codebase, the connection may never be closed out for two threads concurrently decrease the ref counter.
          And the following code just hides the the possible bug that there are more decCount ops than incCount ops in the code.

             void decCount() {
                if (refCount > 0) {
                  --refCount;
                }
             }
          

          In my opinion,we should expose the bug as early as possible.
          (1) When the count goes negative, we should print warn log or throw an exception.
          (2) When someone forgets to call the decCount, and the ref count always be positive , the connection never be closed. we can find this problem from the output of jstack.

          I think the change doesn't introduce a new class of bug.

          liushaohui Shaohui Liu added a comment - apurtell I really think we need to preserve the earlier logic I'm sorry that i didn't get your meanings? is the earlier logic just the current codebase? The current codebase, the connection may never be closed out for two threads concurrently decrease the ref counter. And the following code just hides the the possible bug that there are more decCount ops than incCount ops in the code. void decCount() { if (refCount > 0) { --refCount; } } In my opinion,we should expose the bug as early as possible. (1) When the count goes negative, we should print warn log or throw an exception. (2) When someone forgets to call the decCount, and the ref count always be positive , the connection never be closed. we can find this problem from the output of jstack. I think the change doesn't introduce a new class of bug.

          > is the earlier logic just the current codebase?

          What I mean by that is this:

              void decCount() {
                if (refCount > 0) {
                  --refCount;
                }
              }
          

          This code does not allow the reference count to go below 0, which is what isZeroReference expects. The latest patch allows the reference count to go below 0 without changing isZeroReference or its callers to handle cases where the refcount is negative.

          > When the count goes negative, we should print warn log or throw an exception.

          Throwing an exception after insuring the connection is cleaned up is good, that would address the concern here.

          apurtell Andrew Kyle Purtell added a comment - > is the earlier logic just the current codebase? What I mean by that is this: void decCount() { if (refCount > 0) { --refCount; } } This code does not allow the reference count to go below 0, which is what isZeroReference expects. The latest patch allows the reference count to go below 0 without changing isZeroReference or its callers to handle cases where the refcount is negative. > When the count goes negative, we should print warn log or throw an exception. Throwing an exception after insuring the connection is cleaned up is good, that would address the concern here.
          liushaohui Shaohui Liu added a comment -

          Updates for apurtell's review

          liushaohui Shaohui Liu added a comment - Updates for apurtell 's review
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12661133/HBASE-11685-trunk-v5.diff
          against trunk revision .
          ATTACHMENT ID: 12661133

          +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.TestMetaTableAccessorNoCluster
          org.apache.hadoop.hbase.security.token.TestZKSecretWatcher

          -1 core zombie tests. There are 1 zombie test(s):

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//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/12661133/HBASE-11685-trunk-v5.diff against trunk revision . ATTACHMENT ID: 12661133 +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.TestMetaTableAccessorNoCluster org.apache.hadoop.hbase.security.token.TestZKSecretWatcher -1 core zombie tests . There are 1 zombie test(s): Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10395//console This message is automatically generated.
          liushaohui Shaohui Liu added a comment -

          Fix the failed test TestMetaTableAccessorNoCluster.
          And It seems that the other failed test TestZKSecretWatcher has no relations with this patch.

          liushaohui Shaohui Liu added a comment - Fix the failed test TestMetaTableAccessorNoCluster. And It seems that the other failed test TestZKSecretWatcher has no relations with this patch.
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff
          against trunk revision .
          ATTACHMENT ID: 12661161

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

          +1 tests included. The patch appears to include 3 new or modified tests.

          +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.TestRegionRebalancing

          -1 core zombie tests. There are 1 zombie test(s): at org.apache.hadoop.hbase.client.TestHCM.testClusterStatus(TestHCM.java:250)

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//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/12661161/HBASE-11685-trunk-v6.diff against trunk revision . ATTACHMENT ID: 12661161 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +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.TestRegionRebalancing -1 core zombie tests . There are 1 zombie test(s): at org.apache.hadoop.hbase.client.TestHCM.testClusterStatus(TestHCM.java:250) Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/10398//console This message is automatically generated.

          v6 patch lgtm. Any comment or concerns nkeywal or stack?

          apurtell Andrew Kyle Purtell added a comment - v6 patch lgtm. Any comment or concerns nkeywal or stack ?

          + refCount = new AtomicInteger(1);
          Could be replaced by a simple set, this would allow refCount to be final. It's theoretically better.

          I wonder if the "throw new RuntimeException("Negative ref count of connection: " + this;" cannot have a race condition with this set to 1 in the finalize?
          It could be simple to simply log a warning?

          nkeywal Nicolas Liochon added a comment - + refCount = new AtomicInteger(1); Could be replaced by a simple set, this would allow refCount to be final. It's theoretically better. I wonder if the "throw new RuntimeException("Negative ref count of connection: " + this;" cannot have a race condition with this set to 1 in the finalize? It could be simple to simply log a warning?

          The problem with only logging a warning is we have code that expects the count never to go below zero and this patch is not changing callers of isZeroReference. Though it suppose isZeroReference could be updated here to return true if <= 0

          apurtell Andrew Kyle Purtell added a comment - The problem with only logging a warning is we have code that expects the count never to go below zero and this patch is not changing callers of isZeroReference. Though it suppose isZeroReference could be updated here to return true if <= 0

          Actually, there is no bug here: increments and decrements are done in synchronized (CONNECTION_INSTANCES) , so they are already atomic. There is no need to use atomicInteger here (a comment on the sync requirement would be welcome however )

          The exception is the finalize (before and after the patch): there is a set without synchronization.
          At the end, I would propose to just remove the finalize.. It's safer and makes the code simpler to read. We cannot rely on a finalize to free resources anyway.

          nkeywal Nicolas Liochon added a comment - Actually, there is no bug here: increments and decrements are done in synchronized (CONNECTION_INSTANCES) , so they are already atomic. There is no need to use atomicInteger here (a comment on the sync requirement would be welcome however ) The exception is the finalize (before and after the patch): there is a set without synchronization. At the end, I would propose to just remove the finalize.. It's safer and makes the code simpler to read. We cannot rely on a finalize to free resources anyway.
          chia7712 Chia-Ping Tsai added a comment -

          The ref count is removed in HBase 2.0 (see HBASE-14787). Shall we close this jira? Or change the version to branch-1 ?

          chia7712 Chia-Ping Tsai added a comment - The ref count is removed in HBase 2.0 (see HBASE-14787 ). Shall we close this jira? Or change the version to branch-1 ?
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          0 patch 0m 6s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.3.0/precommit-patchnames for instructions.
          -1 patch 0m 10s HBASE-11685 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.3.0/precommit-patchnames for help.



          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. 0 patch 0m 6s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.3.0/precommit-patchnames for instructions. -1 patch 0m 10s HBASE-11685 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.3.0/precommit-patchnames for help. Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff JIRA Issue HBASE-11685 Console output https://builds.apache.org/job/PreCommit-HBASE-Build/6465/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          stack Michael Stack added a comment -

          Made it a branch-1 issue.

          stack Michael Stack added a comment - Made it a branch-1 issue.
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          0 patch 0m 2s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.4.0/precommit-patchnames for instructions.
          -1 patch 0m 6s HBASE-11685 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.4.0/precommit-patchnames for help.



          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. 0 patch 0m 2s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.4.0/precommit-patchnames for instructions. -1 patch 0m 6s HBASE-11685 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.4.0/precommit-patchnames for help. Subsystem Report/Notes JIRA Issue HBASE-11685 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff Console output https://builds.apache.org/job/PreCommit-HBASE-Build/7431/console Powered by Apache Yetus 0.4.0 http://yetus.apache.org This message was automatically generated.
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          0 patch 0m 3s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.8.0/precommit-patchnames for instructions.
          -1 patch 0m 5s HBASE-11685 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.8.0/precommit-patchnames for help.



          This message was automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. 0 patch 0m 3s The patch file was not named according to hbase's naming conventions. Please see https://yetus.apache.org/documentation/0.8.0/precommit-patchnames for instructions. -1 patch 0m 5s HBASE-11685 does not apply to master. Rebase required? Wrong Branch? See https://yetus.apache.org/documentation/0.8.0/precommit-patchnames for help. Subsystem Report/Notes JIRA Issue HBASE-11685 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12661161/HBASE-11685-trunk-v6.diff Console output https://builds.apache.org/job/PreCommit-HBASE-Build/15841/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.

          No progress, cancelling patch

          apurtell Andrew Kyle Purtell added a comment - No progress, cancelling patch

          Any progress here? Or unschedule it? Or close it?

          apurtell Andrew Kyle Purtell added a comment - Any progress here? Or unschedule it? Or close it?

          People

            Unassigned Unassigned
            liushaohui Shaohui Liu
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: