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

HBaseAdmin should perform validation of connection it holds

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.92.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Through HBASE-3777, HConnectionManager reuses the connection to HBase servers.
      One challenge, discovered in troubleshooting HBASE-4052, is how we invalidate connection(s) to server which gets restarted.
      There're at least two ways.
      1. HConnectionManager utilizes background thread(s) to periodically perform validation of connections in HBASE_INSTANCES and remove stale connection(s).
      2. Allow HBaseClient (including HBaseAdmin) to provide feedback to HConnectionManager.

      The solution can be a combination of both of the above.

      1. 4087-v4.txt
        6 kB
        Ted Yu
      2. 4087-v3.txt
        5 kB
        Ted Yu
      3. 4087-v2.txt
        7 kB
        Ted Yu
      4. 4087.txt
        6 kB
        Ted Yu

        Activity

        Hide
        srivas M. C. Srivas added a comment -

        I prefer option 2. When the HBaseClient fails to connect to the master, make it come back once more to the HConnectionManager (via a different API that also provides the bad handle), so the HConnectionManager can reset its cache. Periodic checks in a bg thread is not scalable (causes needless load on the server), and does not really work. For example, what happens if the server crashed right after the check? We would still need option 2 to say "invalidate now and get me a new handle" in the client in order to make immediate progress.

        Show
        srivas M. C. Srivas added a comment - I prefer option 2. When the HBaseClient fails to connect to the master, make it come back once more to the HConnectionManager (via a different API that also provides the bad handle), so the HConnectionManager can reset its cache. Periodic checks in a bg thread is not scalable (causes needless load on the server), and does not really work. For example, what happens if the server crashed right after the check? We would still need option 2 to say "invalidate now and get me a new handle" in the client in order to make immediate progress.
        Hide
        stack stack added a comment -

        -1 on option 1. Why can't we just let the exception bubble up when master connection is bad. I suppose this means that we need to invalidate the object that is in cache and renew it on exception – but not all IOEs?

        Show
        stack stack added a comment - -1 on option 1. Why can't we just let the exception bubble up when master connection is bad. I suppose this means that we need to invalidate the object that is in cache and renew it on exception – but not all IOEs?
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        First attempt at solving this issue.
        When connection.getMaster() throws UndeclaredThrowableException (corresponding to Connection refused error), we close the stale connection and reestablish new connection.
        I have to declare HBaseAdmin.connection as not final.

        TestMasterRestartAfterDisablingTable (from HBASE-4052) passes with this patch.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - First attempt at solving this issue. When connection.getMaster() throws UndeclaredThrowableException (corresponding to Connection refused error), we close the stale connection and reestablish new connection. I have to declare HBaseAdmin.connection as not final. TestMasterRestartAfterDisablingTable (from HBASE-4052 ) passes with this patch.
        Hide
        srivas M. C. Srivas added a comment -

        There seems to be a race in HConnectionManager#deleteConnection

        if (connection.isZeroReference() || staleConnection)

        { delete the connection ... }

        When staleConnection is true, the code should wait for reference to become zero, else the connection gets deleted from underneath another thread.

        Show
        srivas M. C. Srivas added a comment - There seems to be a race in HConnectionManager#deleteConnection if (connection.isZeroReference() || staleConnection) { delete the connection ... } When staleConnection is true, the code should wait for reference to become zero, else the connection gets deleted from underneath another thread.
        Hide
        stack stack added a comment -

        Why we getting UndeclaredThrowableException? Whats being thrown that is not part of the interface? Connection refused? Can we add that?

        Any chance of a test Ted? A unit test where we do some function on master with HBA and then we kill and start new master? See if HBA keeps going?

        Show
        stack stack added a comment - Why we getting UndeclaredThrowableException? Whats being thrown that is not part of the interface? Connection refused? Can we add that? Any chance of a test Ted? A unit test where we do some function on master with HBA and then we kill and start new master? See if HBA keeps going?
        Hide
        stack stack added a comment -

        This issue is ugly. This patch is incomplete no? Any use of connection needs to catch this undeclared exception and be able to mark it bad with HConnectionManager. Right?

        Show
        stack stack added a comment - This issue is ugly. This patch is incomplete no? Any use of connection needs to catch this undeclared exception and be able to mark it bad with HConnectionManager. Right?
        Hide
        srivas M. C. Srivas added a comment -

        @stack: yes, its ugly but necessary. If the multi-master failover needs to work, the hbase client must behave in a "nfs hard-mount" manner, ie, keep trying till a master comes online. Perhaps the retry can be buried under the API? The retry should fail if ZK itself is not reachable.

        Show
        srivas M. C. Srivas added a comment - @stack: yes, its ugly but necessary. If the multi-master failover needs to work, the hbase client must behave in a "nfs hard-mount" manner, ie, keep trying till a master comes online. Perhaps the retry can be buried under the API? The retry should fail if ZK itself is not reachable.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Patch version 2 addresses Srivas' comment.
        It is incomplete. I want to get confirmation that similar changes should be made to the files I mentioned in my email to dev@

        The undeclaredThrowable field of UndeclaredThrowableException is an IOException, similar to the following:

        java.io.IOException: Call to /192.168.2.107:60697 failed on local exception: java.io.IOException: Connection reset by peer
        

        A test for this JIRA has been added in HBASE-4052: TestMasterRestartAfterDisablingTable. I used that test to obtain the above information.
        Without fix for this JIRA, TestMasterRestartAfterDisablingTable wouldn't pass.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Patch version 2 addresses Srivas' comment. It is incomplete. I want to get confirmation that similar changes should be made to the files I mentioned in my email to dev@ The undeclaredThrowable field of UndeclaredThrowableException is an IOException, similar to the following: java.io.IOException: Call to /192.168.2.107:60697 failed on local exception: java.io.IOException: Connection reset by peer A test for this JIRA has been added in HBASE-4052 : TestMasterRestartAfterDisablingTable. I used that test to obtain the above information. Without fix for this JIRA, TestMasterRestartAfterDisablingTable wouldn't pass.
        Hide
        srivas M. C. Srivas added a comment -

        Hi Ted, the two versions seem very similar. I was expecting some code like the following in HConnectionManager#deleteConnection:

             synchronized (HBASE_INSTANCES) {
               HConnectionImplementation connection = HBASE_INSTANCES
                   .get(connectionKey);
               if (connection != null) {
                 connection.decCount();
        +        HBASE_INSTANCES.notify();
                 if (connection.isZeroReference() || staleConnection) {
        +          while (! connection.isZeroReference()) {
        +             HBASE_INSTANCES.wait();
        +          }
                   HBASE_INSTANCES.remove(connectionKey);
                   connection.close(stopProxy);
                 } else if (stopProxy) {
        
        

        Is there a problem with the call to HBASE_INSTANCES.wait()? I mean, will it hang the system as the waiting thread may hold other synchronized-blocks while calling into HConnectionManager#deleteConnection? If so, my change above is bogus, and we will need some sort of "connection#state" to keep track of the fact that the connection is bad, but there are still references to it.

        Show
        srivas M. C. Srivas added a comment - Hi Ted, the two versions seem very similar. I was expecting some code like the following in HConnectionManager#deleteConnection: synchronized (HBASE_INSTANCES) { HConnectionImplementation connection = HBASE_INSTANCES .get(connectionKey); if (connection != null) { connection.decCount(); + HBASE_INSTANCES.notify(); if (connection.isZeroReference() || staleConnection) { + while (! connection.isZeroReference()) { + HBASE_INSTANCES.wait(); + } HBASE_INSTANCES.remove(connectionKey); connection.close(stopProxy); } else if (stopProxy) { Is there a problem with the call to HBASE_INSTANCES.wait()? I mean, will it hang the system as the waiting thread may hold other synchronized-blocks while calling into HConnectionManager#deleteConnection? If so, my change above is bogus, and we will need some sort of "connection#state" to keep track of the fact that the connection is bad, but there are still references to it.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        First, waiting for reference count to reach zero wasn't part of the original semantics of obtaining connection.
        Suppose there were two clients A and B. Client A is waiting in the new while loop for reference count to reach zero. What if client B had a bug and crashed ?

        Patch version 2 directly removes the stale connection when the first client tried to connect to a dead server. Other clients wouldn't remove the new connection handed out to the first client. Since they share same config with the first client, they would retrieve the new connection directly.

        Bookkeeping using connection#state would also work. But the above handling seems cleaner.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - First, waiting for reference count to reach zero wasn't part of the original semantics of obtaining connection. Suppose there were two clients A and B. Client A is waiting in the new while loop for reference count to reach zero. What if client B had a bug and crashed ? Patch version 2 directly removes the stale connection when the first client tried to connect to a dead server. Other clients wouldn't remove the new connection handed out to the first client. Since they share same config with the first client, they would retrieve the new connection directly. Bookkeeping using connection#state would also work. But the above handling seems cleaner.
        Hide
        stack stack added a comment -

        @Srivas Yes, we should bury it under the API if possible; add an isGood method to Connection Interface. When connection goes bad, it marks itself so and recourse is to go get a new one (This may not be possible).

        @Ted so we need to change our APIs so that they all can catch IOE? How then to tell difference between retryable IOE and one that makes the connection go 'bad'? We have already the Retryable exception interface that we have some forms of IOE implement.

        Is it true that the code previous to hbase-3777 had this same issue?

        Show
        stack stack added a comment - @Srivas Yes, we should bury it under the API if possible; add an isGood method to Connection Interface. When connection goes bad, it marks itself so and recourse is to go get a new one (This may not be possible). @Ted so we need to change our APIs so that they all can catch IOE? How then to tell difference between retryable IOE and one that makes the connection go 'bad'? We have already the Retryable exception interface that we have some forms of IOE implement. Is it true that the code previous to hbase-3777 had this same issue?
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        In case of server outage/restart, declared exceptions wouldn't be thrown:

          public HBaseAdmin(Configuration c)
          throws MasterNotRunningException, ZooKeeperConnectionException {
        

        I need to dig deeper into current failure in TestMasterFailover to answer the last question above.

        I could use the following structure in place of current:

          Foo result = new RetryOnce<Foo>() {
            Foo doIt(HConnection connection) {
              return connection.foo();
            }
          }.run();
        
          abstract class RetryOnce<T> {
            T run() {
              try {
                return doIt(connection);
              }
              catch (UndeclaredThrowableException ute) {
                HConnectionManager.deleteStaleConnection(connection);
                connection = HConnectionManager.getConnection(conf);
                return doIt(connection);
              }
            }
            abstract T doIt(HConnection connection);
          }
        

        I believe Karthick proposed similar construct when he was implementing HBASE-3777 but the response was lukewarm. Further only two lines were saved in the above construct.

        The challenge here is that we should expect client to cache the connection handed out to them, making burying it under the API very hard to achieve.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - In case of server outage/restart, declared exceptions wouldn't be thrown: public HBaseAdmin(Configuration c) throws MasterNotRunningException, ZooKeeperConnectionException { I need to dig deeper into current failure in TestMasterFailover to answer the last question above. I could use the following structure in place of current: Foo result = new RetryOnce<Foo>() { Foo doIt(HConnection connection) { return connection.foo(); } }.run(); abstract class RetryOnce<T> { T run() { try { return doIt(connection); } catch (UndeclaredThrowableException ute) { HConnectionManager.deleteStaleConnection(connection); connection = HConnectionManager.getConnection(conf); return doIt(connection); } } abstract T doIt(HConnection connection); } I believe Karthick proposed similar construct when he was implementing HBASE-3777 but the response was lukewarm. Further only two lines were saved in the above construct. The challenge here is that we should expect client to cache the connection handed out to them, making burying it under the API very hard to achieve.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        There is one enhancement I can make on top of patch version 2.
        We can add the following method to HCM:

        HConnection replaceStaleConnection(this.conf);
        

        This method would retrieve a new connection and replace the old connection corresponding to the passed conf object with the new one.
        Such optimization is minor, considering that we have to wrap every connection call in client in a similar manner to that in patch version 2.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - There is one enhancement I can make on top of patch version 2. We can add the following method to HCM: HConnection replaceStaleConnection( this .conf); This method would retrieve a new connection and replace the old connection corresponding to the passed conf object with the new one. Such optimization is minor, considering that we have to wrap every connection call in client in a similar manner to that in patch version 2.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        In TRUNK build 2027, TestMasterFailover passed.
        This means we don't need to perform full-scale surgery.

        Keeping the changes in HCM along with HBA constructor should be enough to unblock test in HBASE-4052.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - In TRUNK build 2027, TestMasterFailover passed. This means we don't need to perform full-scale surgery. Keeping the changes in HCM along with HBA constructor should be enough to unblock test in HBASE-4052 .
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Version 3 narrows the scope of change in HBA.

        TestMasterRestartAfterDisablingTable passes based on this patch.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Version 3 narrows the scope of change in HBA. TestMasterRestartAfterDisablingTable passes based on this patch.
        Hide
        srivas M. C. Srivas added a comment -

        @Ted:

        First, waiting for reference count to reach zero wasn't part of the original semantics of obtaining connection. Suppose there were two clients A and B. Client A is waiting in the new while loop for reference count to reach zero. What if client B had a bug and crashed ?

        If many threads share a connection, the reference count protects the connection from being destroyed until the last reference is dropped. We need to continue to preserve that guarantee. With the change being proposed, if any thread calls HConnectionManager#dropStaleConnection, the connection gets destroyed from underneath other threads. The sequence of steps that causes this is as follows:

        1. threads T1 and T2 both get connections using the same conf, thus are returned the same connection.
        2. T1 gets a failure, so it deletes the stale connection and creates a new one (thus HBASE_INSTANCES.get() will return the new good connection, since the HConnectionKey is identical).
        3. T2 still has a handle on the old one, and gets a failure, and calls deleteStaleConnection.
        4. deleteStaleConnection destroys the good connection created in step 2, from underneath T1.

        So either we wait for the last ref to become zero, or set some sort of connection#state == INVALID if you'd rather not wait. I think the second choice (not waiting) seems better.

        I didn't follow your second comment about client A and client B. Both are in the same JVM, so if one crashes, the other would too, would it not?

        Show
        srivas M. C. Srivas added a comment - @Ted: First, waiting for reference count to reach zero wasn't part of the original semantics of obtaining connection. Suppose there were two clients A and B. Client A is waiting in the new while loop for reference count to reach zero. What if client B had a bug and crashed ? If many threads share a connection, the reference count protects the connection from being destroyed until the last reference is dropped. We need to continue to preserve that guarantee. With the change being proposed, if any thread calls HConnectionManager#dropStaleConnection, the connection gets destroyed from underneath other threads. The sequence of steps that causes this is as follows: 1. threads T1 and T2 both get connections using the same conf, thus are returned the same connection. 2. T1 gets a failure, so it deletes the stale connection and creates a new one (thus HBASE_INSTANCES.get() will return the new good connection, since the HConnectionKey is identical). 3. T2 still has a handle on the old one, and gets a failure, and calls deleteStaleConnection. 4. deleteStaleConnection destroys the good connection created in step 2, from underneath T1. So either we wait for the last ref to become zero, or set some sort of connection#state == INVALID if you'd rather not wait. I think the second choice (not waiting) seems better. I didn't follow your second comment about client A and client B. Both are in the same JVM, so if one crashes, the other would too, would it not?
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        4. deleteStaleConnection destroys the good connection created in step 2, from underneath T1.

        In patch versions 2 and 3, I have:

          public static void deleteStaleConnection(HConnection connection) {
        

        When T2 tries to destroy old handle, nothing would happen because the loop in deleteConnection() wouldn't find the old connection.

        My description about client B was not accurate. But we now agree that not waiting is a better solution.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - 4. deleteStaleConnection destroys the good connection created in step 2, from underneath T1. In patch versions 2 and 3, I have: public static void deleteStaleConnection(HConnection connection) { When T2 tries to destroy old handle, nothing would happen because the loop in deleteConnection() wouldn't find the old connection. My description about client B was not accurate. But we now agree that not waiting is a better solution.
        Hide
        srivas M. C. Srivas added a comment -

        Missed that, thanks for pointing it out. Looks like we don't need a connection#state either.

        Show
        srivas M. C. Srivas added a comment - Missed that, thanks for pointing it out. Looks like we don't need a connection#state either.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        If I get +1 vote(s) for version 3, I would check in that fix which would unblock HBASE-4052.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - If I get +1 vote(s) for version 3, I would check in that fix which would unblock HBASE-4052 .
        Hide
        srivas M. C. Srivas added a comment -

        +1 for the stuff about clearing the stale connection inside the HConnectionManager. Whether we should retry only once, or in a loop until we get a good connection ... I will defer to Stack.

        Show
        srivas M. C. Srivas added a comment - +1 for the stuff about clearing the stale connection inside the HConnectionManager. Whether we should retry only once, or in a loop until we get a good connection ... I will defer to Stack.
        Hide
        stack stack added a comment -

        This looks good. What you think about the retry Srivas suggests? And is this patch for 0.90 or for trunk? If trunk, can we change the signature to be IOE so exception doesn't arrive at client as a UndeclaredThrowableException?

        Show
        stack stack added a comment - This looks good. What you think about the retry Srivas suggests? And is this patch for 0.90 or for trunk? If trunk, can we change the signature to be IOE so exception doesn't arrive at client as a UndeclaredThrowableException?
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        I will upload a new patch with retry logic.

        If I change the signature to throwing IOE, then I also have to change the signature for this method which constructs a new HBA:

          public static void checkHBaseAvailable(Configuration conf)
          throws MasterNotRunningException, ZooKeeperConnectionException {
        

        I wanted to limit the scope of the patch since I only handle the exception in the ctor.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - I will upload a new patch with retry logic. If I change the signature to throwing IOE, then I also have to change the signature for this method which constructs a new HBA: public static void checkHBaseAvailable(Configuration conf) throws MasterNotRunningException, ZooKeeperConnectionException { I wanted to limit the scope of the patch since I only handle the exception in the ctor.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Patch version 4 added retry logic.

        TestMasterRestartAfterDisablingTable passed.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Patch version 4 added retry logic. TestMasterRestartAfterDisablingTable passed.
        Hide
        stack stack added a comment -

        I think v4 good for now. Go commit Ted.

        Show
        stack stack added a comment - I think v4 good for now. Go commit Ted.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Integrated to TRUNK.

        Thanks for the review Srivas and Stack.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Integrated to TRUNK. Thanks for the review Srivas and Stack.
        Hide
        hudson Hudson added a comment -

        Integrated in HBase-TRUNK #2036 (See https://builds.apache.org/job/HBase-TRUNK/2036/)
        HBASE-4087 HBaseAdmin should perform validation of connection it holds

        tedyu :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
        • /hbase/trunk/CHANGES.txt
        Show
        hudson Hudson added a comment - Integrated in HBase-TRUNK #2036 (See https://builds.apache.org/job/HBase-TRUNK/2036/ ) HBASE-4087 HBaseAdmin should perform validation of connection it holds tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java /hbase/trunk/CHANGES.txt
        Hide
        lars_francke Lars Francke added a comment -

        This issue was closed as part of a bulk closing operation on 2015-11-20. All issues that have been resolved and where all fixVersions have been released have been closed (following discussions on the mailing list).

        Show
        lars_francke Lars Francke added a comment - This issue was closed as part of a bulk closing operation on 2015-11-20. All issues that have been resolved and where all fixVersions have been released have been closed (following discussions on the mailing list).

          People

          • Assignee:
            yuzhihong@gmail.com Ted Yu
            Reporter:
            yuzhihong@gmail.com Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development