HBase
  1. HBase
  2. HBASE-5682

Allow HConnectionImplementation to recover from ZK connection loss (for 0.94 only)

    Details

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

      Description

      Just realized that without this HBASE-4805 is broken.
      I.e. there's no point keeping a persistent HConnection around if it can be rendered permanently unusable if the ZK connection is lost temporarily.
      Note that this is fixed in 0.96 with HBASE-5399 (but that seems to big to backport)

      1. 5682.txt
        5 kB
        Lars Hofhansl
      2. 5682-v2.txt
        6 kB
        Lars Hofhansl
      3. 5682-all.txt
        6 kB
        Lars Hofhansl
      4. 5682-all-v2.txt
        7 kB
        Lars Hofhansl
      5. 5682-all-v3.txt
        12 kB
        Lars Hofhansl
      6. 5682-all-v4.txt
        12 kB
        Lars Hofhansl

        Activity

        Lars Hofhansl created issue -
        Lars Hofhansl made changes -
        Field Original Value New Value
        Fix Version/s 0.90.6 [ 12319200 ]
        Lars Hofhansl made changes -
        Component/s client [ 12312131 ]
        Lars Hofhansl made changes -
        Assignee Lars Hofhansl [ lhofhansl ]
        Lars Hofhansl made changes -
        Fix Version/s 0.94.1 [ 12320257 ]
        Fix Version/s 0.90.6 [ 12319200 ]
        Hide
        Lars Hofhansl added a comment - - edited

        Here's a patch.
        Please have a careful look. I can upload to RB too.

        The idea is that if this is an unmanaged Connection (see HBASE-4805), a ZK connection is re-establish whenever needed (if it was lost before).

        This patch is somewhat more complicated than I'd like, because I did not want to change the behavior for managed (default) connections.
        If we like I can make this the default behavior... Seems much more robust than the current behavior.

        I tested this manually, and the connection (if created with HConnectionManager.createConnection, and hence unmanaged) recovers from loosing both the HBase and ZK connections.

        (Interestingly in plain HBase 0.94 the client never recovers from this - even with the default connection behavior.)

        Show
        Lars Hofhansl added a comment - - edited Here's a patch. Please have a careful look. I can upload to RB too. The idea is that if this is an unmanaged Connection (see HBASE-4805 ), a ZK connection is re-establish whenever needed (if it was lost before). This patch is somewhat more complicated than I'd like, because I did not want to change the behavior for managed (default) connections. If we like I can make this the default behavior... Seems much more robust than the current behavior. I tested this manually, and the connection (if created with HConnectionManager.createConnection, and hence unmanaged) recovers from loosing both the HBase and ZK connections. (Interestingly in plain HBase 0.94 the client never recovers from this - even with the default connection behavior.)
        Lars Hofhansl made changes -
        Attachment 5682.txt [ 12520696 ]
        Hide
        Lars Hofhansl added a comment - - edited

        I am willing to sink the 0.94.0rc for this.

        Show
        Lars Hofhansl added a comment - - edited I am willing to sink the 0.94.0rc for this.
        Lars Hofhansl made changes -
        Summary Add retry logic in HConnectionImplementation#resetZooKeeperTrackers (port to 0.94) Add retry logic in HConnectionImplementation#resetZooKeeperTrackers (for 0.94 only)
        Lars Hofhansl made changes -
        Parent HBASE-5153 [ 12537717 ]
        Issue Type Sub-task [ 7 ] Improvement [ 4 ]
        Hide
        Lars Hofhansl added a comment -

        A little bit more background:
        I put HBASE-4805 in place to be able to use the standard HBase client in long running app server processes. The idea is that one can manage an HConnection and a ThreadPool per app server and then cheaply create HTables when needed.
        For that setup it is vital that the HConnection does not become unusable when there are temporary network outages, or that HBase cluster is temporarily taken down.
        In that case the clients should timeout quickly to allow the application to react to it, and if the network/cluster has recovered the application should be able to recover.

        Show
        Lars Hofhansl added a comment - A little bit more background: I put HBASE-4805 in place to be able to use the standard HBase client in long running app server processes. The idea is that one can manage an HConnection and a ThreadPool per app server and then cheaply create HTables when needed. For that setup it is vital that the HConnection does not become unusable when there are temporary network outages, or that HBase cluster is temporarily taken down. In that case the clients should timeout quickly to allow the application to react to it, and if the network/cluster has recovered the application should be able to recover.
        Hide
        Lars Hofhansl added a comment -

        Slightly better patch.
        No need to wait waitForRootRegion in ZK longer than RecoverableZookeeper tries.

        With recovery is clean. The client can control via timeouts how soon it would it would a connection problem up to the application layer.

        Since this patch is only against 0.94 I'll run tests locally.

        Show
        Lars Hofhansl added a comment - Slightly better patch. No need to wait waitForRootRegion in ZK longer than RecoverableZookeeper tries. With recovery is clean. The client can control via timeouts how soon it would it would a connection problem up to the application layer. Since this patch is only against 0.94 I'll run tests locally.
        Lars Hofhansl made changes -
        Attachment 5682-v2.txt [ 12520762 ]
        Hide
        Lars Hofhansl added a comment -

        In fact this is what HBASE-5153 should have been.

        Show
        Lars Hofhansl added a comment - In fact this is what HBASE-5153 should have been.
        Lars Hofhansl made changes -
        Summary Add retry logic in HConnectionImplementation#resetZooKeeperTrackers (for 0.94 only) Allow HConnectionImplementation to recover from ZK connection loss (for 0.94 only)
        Hide
        Ted Yu added a comment -

        For abort():

        +            if (managed) {
        +              // if the connection is managed attempt to reconnect immediately
        +              ensureZookeeperTrackers();
        

        the condition for calling ensureZookeeperTrackers() is different from other calls in the patch. Please explain.

        +    private synchronized void ensureZookeeperTrackers()
                 throws ZooKeeperConnectionException{
        

        Please add a space before the right curly brace.

        Show
        Ted Yu added a comment - For abort(): + if (managed) { + // if the connection is managed attempt to reconnect immediately + ensureZookeeperTrackers(); the condition for calling ensureZookeeperTrackers() is different from other calls in the patch. Please explain. + private synchronized void ensureZookeeperTrackers() throws ZooKeeperConnectionException{ Please add a space before the right curly brace.
        Hide
        Lars Hofhansl added a comment -

        Thanks Ted.

        1. the condition is different, because that is what it did before. I.e. if the connection is managed the trackers are setup only at construction and during abort in the specific case of SessionExpiredException. If the connection is unmanaged on the other hand the trackers are rechecked before they are needed and hence abort becomes a no-op for any KeeperExcepion. Hence the condition is exactly reversed. This part is the key of the patch actually.
        2. The space wasn't there before. I actually had the space added and then removed it again I'll add it back.
        Show
        Lars Hofhansl added a comment - Thanks Ted. the condition is different, because that is what it did before. I.e. if the connection is managed the trackers are setup only at construction and during abort in the specific case of SessionExpiredException. If the connection is unmanaged on the other hand the trackers are rechecked before they are needed and hence abort becomes a no-op for any KeeperExcepion. Hence the condition is exactly reversed. This part is the key of the patch actually. The space wasn't there before. I actually had the space added and then removed it again I'll add it back.
        Hide
        Lars Hofhansl added a comment -

        The gist of this change is that (1) the ZK connection is re-checked in all calls where it is needed and re-established if needed and (2) if the connection is down the client can find out quickly (by setting timeouts accordingly) and report via IOException to the calling thread.
        This is only done for unmanaged HConnections (those that were created with HConnectionManager.createConnection(...) and are hence not reference counted. Reference counted HConnctions are treated as before.)

        This is needed to safely use the HConnection is a multithreaded long-lived AppServer setting.

        (In my tests I found that even 0.96 needs some more work here, but that's for a different jira.)

        Show
        Lars Hofhansl added a comment - The gist of this change is that (1) the ZK connection is re-checked in all calls where it is needed and re-established if needed and (2) if the connection is down the client can find out quickly (by setting timeouts accordingly) and report via IOException to the calling thread. This is only done for unmanaged HConnections (those that were created with HConnectionManager.createConnection(...) and are hence not reference counted. Reference counted HConnctions are treated as before.) This is needed to safely use the HConnection is a multithreaded long-lived AppServer setting. (In my tests I found that even 0.96 needs some more work here, but that's for a different jira.)
        Hide
        Lars Hofhansl added a comment -

        v2 passes all tests locally.

        Show
        Lars Hofhansl added a comment - v2 passes all tests locally.
        Hide
        Ted Yu added a comment -

        +1 on patch.

        Show
        Ted Yu added a comment - +1 on patch.
        Hide
        Lars Hofhansl added a comment -

        Thanks Ted.
        The last question is: Should we do this for all HConnection (not just for unmanaged ones)? It means that HConnection would be able to recover from loss of ZK connection and the abort() method would only clear out the ZK trackers and never close or abort he connection. I'd be in favor of that.

        @Ram and @Jieshan: Since would a more robust version of HBASE-5153, could you have a look at this?

        Show
        Lars Hofhansl added a comment - Thanks Ted. The last question is: Should we do this for all HConnection (not just for unmanaged ones)? It means that HConnection would be able to recover from loss of ZK connection and the abort() method would only clear out the ZK trackers and never close or abort he connection. I'd be in favor of that. @Ram and @Jieshan: Since would a more robust version of HBASE-5153 , could you have a look at this?
        Lars Hofhansl made changes -
        Fix Version/s 0.94.0 [ 12316419 ]
        Fix Version/s 0.94.1 [ 12320257 ]
        Hide
        Ted Yu added a comment -

        Application to other HConnection makes sense.

        Show
        Ted Yu added a comment - Application to other HConnection makes sense.
        Hide
        stack added a comment -

        This is a perversion.

        If we pass in a connection from outside, down in the guts, do special handling that makes the connection and zookeeper handling do reconnect. Its like we should be passing an Interface made at a higher-level of abstraction and then in the implementation, it did this fixup when connection breaks.

        With that out of the way, do whatever you need to make it work. Patch looks fine. How did you test. Would it be hard to make a unit test of it. A unit test would be good codifying this perversion since it will be brittle being not whats expected.

        I'm against changing the behavior of the default case in 0.92/0.94. I'm interested in problems you see in hbase-5153 or issues you have w/ the implementation there that being the 0.96 client.

        Show
        stack added a comment - This is a perversion. If we pass in a connection from outside, down in the guts, do special handling that makes the connection and zookeeper handling do reconnect. Its like we should be passing an Interface made at a higher-level of abstraction and then in the implementation, it did this fixup when connection breaks. With that out of the way, do whatever you need to make it work. Patch looks fine. How did you test. Would it be hard to make a unit test of it. A unit test would be good codifying this perversion since it will be brittle being not whats expected. I'm against changing the behavior of the default case in 0.92/0.94. I'm interested in problems you see in hbase-5153 or issues you have w/ the implementation there that being the 0.96 client.
        Hide
        Lars Hofhansl added a comment -

        Here's a patch that always attempts reconnecting to ZK when a ZK connection is needed.

        Show
        Lars Hofhansl added a comment - Here's a patch that always attempts reconnecting to ZK when a ZK connection is needed.
        Lars Hofhansl made changes -
        Attachment 5682-all.txt [ 12520804 ]
        Hide
        Lars Hofhansl added a comment -

        "perversion" is hard word. It is just rechecking before each use whether the trackers are still usable. The timeout is handled through the HConnection's abort().

        The testing I've done:

        1. ZK down, HBase down, start a client. Then start ZK, then HBase.
        2. ZK up, HBase down, start client. Then start HBase
        3. both ZK and HBase up, start client, kill HBase, restart HBase
        4. both ZK and HBase up, start client, kill ZK and HBase restart

        The client just create a new HTable and then tries to get some rows in a loop.
        In all cases the client should successfully be able to reconnect when both ZK and HBase are up.

        The problem I have seen in 0.94/0.92 without this patch even with managed connections is that after HConnection times out, it is unusable and even getting a new HTable does not fix the problem since behind the scenes the same HConnection is retrieved.

        Will think about an automated test. Do you like the version better that always does the recheck (and hence all the conditional for "managed" go away)?

        Show
        Lars Hofhansl added a comment - "perversion" is hard word. It is just rechecking before each use whether the trackers are still usable. The timeout is handled through the HConnection's abort(). The testing I've done: ZK down, HBase down, start a client. Then start ZK, then HBase. ZK up, HBase down, start client. Then start HBase both ZK and HBase up, start client, kill HBase, restart HBase both ZK and HBase up, start client, kill ZK and HBase restart The client just create a new HTable and then tries to get some rows in a loop. In all cases the client should successfully be able to reconnect when both ZK and HBase are up. The problem I have seen in 0.94/0.92 without this patch even with managed connections is that after HConnection times out, it is unusable and even getting a new HTable does not fix the problem since behind the scenes the same HConnection is retrieved. Will think about an automated test. Do you like the version better that always does the recheck (and hence all the conditional for "managed" go away)?
        Hide
        Lars Hofhansl added a comment -

        The more I look at, the more I do like the patch that changes the behavior in all cases.
        It's simple and low risk: Just recheck the ZK trackers before they are needed.

        Show
        Lars Hofhansl added a comment - The more I look at, the more I do like the patch that changes the behavior in all cases. It's simple and low risk: Just recheck the ZK trackers before they are needed.
        Hide
        stack added a comment -

        The problem I have seen in 0.94/0.92 without this patch even with managed connections is that after HConnection times out, it is unusable and even getting a new HTable does not fix the problem since behind the scenes the same HConnection is retrieved.

        Didn't we add a check for if the connection is bad?

        Will think about an automated test. Do you like the version better that always does the recheck (and hence all the conditional for "managed" go away)?

        How does this work in trunk? In trunk the work has been done so we don't really keep open a zk session any more. For the sake of making tests run smoother, we'll do keep alive on zk session and hold it open 5 minutes and let it go if unused.

        I'm +1 on making our stuff more resilient. Resusing a dud hconnection either because the connection is dead or zk session died is hard to figure.

        How will this change a users's perception about how this stuff is used? If your answer is that it helps in the extreme where the connection goes dead, and thats the only change a user percieves, then lets commit. But we should include a test? If you describe one, I can try help write it?

        You think this should go into 0.92?

        Show
        stack added a comment - The problem I have seen in 0.94/0.92 without this patch even with managed connections is that after HConnection times out, it is unusable and even getting a new HTable does not fix the problem since behind the scenes the same HConnection is retrieved. Didn't we add a check for if the connection is bad? Will think about an automated test. Do you like the version better that always does the recheck (and hence all the conditional for "managed" go away)? How does this work in trunk? In trunk the work has been done so we don't really keep open a zk session any more. For the sake of making tests run smoother, we'll do keep alive on zk session and hold it open 5 minutes and let it go if unused. I'm +1 on making our stuff more resilient. Resusing a dud hconnection either because the connection is dead or zk session died is hard to figure. How will this change a users's perception about how this stuff is used? If your answer is that it helps in the extreme where the connection goes dead, and thats the only change a user percieves, then lets commit. But we should include a test? If you describe one, I can try help write it? You think this should go into 0.92?
        Hide
        stack added a comment -

        I looked at the 'all' patch. Looks good to me. Am interested in how it changes API usage (if at all).

        Show
        stack added a comment - I looked at the 'all' patch. Looks good to me. Am interested in how it changes API usage (if at all).
        Hide
        Lars Hofhansl added a comment -

        I am not envisioning any API changes, just that the HConnection would no longer be ripped from under any HTables where there is a ZK connection loss.

        I ran all tests again, and TestReplication and TestZookeeper have some failures that are related. Looking.

        Show
        Lars Hofhansl added a comment - I am not envisioning any API changes, just that the HConnection would no longer be ripped from under any HTables where there is a ZK connection loss. I ran all tests again, and TestReplication and TestZookeeper have some failures that are related. Looking.
        Hide
        Lars Hofhansl added a comment - - edited

        Found the problem.
        The ClusterId could remain null permanently if HConnection.getZookeeperWatcher() was called. That would initialize HConnectionImplementation.zookeeper, and hence not reset clusterid in ensureZookeeperTrackers.
        TestZookeeper.testClientSessionExpired does that.

        Also in TestZookeeper.testClientSessionExpired the state might be CONNECTING rather than CONNECTED depending on timing.

        Upon inspection I also made clusterId, rootRegionTracker, masterAddressTracker, and zooKeeper volatile, because they can be modified by a different thread, but are not exclusively accessed in a synchronized block (existing problem).

        New patch that fixes the problem, passes all tests.

        TestZookeeper seems to have good coverage. If I can think of more tests, I'll add them there.

        Show
        Lars Hofhansl added a comment - - edited Found the problem. The ClusterId could remain null permanently if HConnection.getZookeeperWatcher() was called. That would initialize HConnectionImplementation.zookeeper, and hence not reset clusterid in ensureZookeeperTrackers. TestZookeeper.testClientSessionExpired does that. Also in TestZookeeper.testClientSessionExpired the state might be CONNECTING rather than CONNECTED depending on timing. Upon inspection I also made clusterId, rootRegionTracker, masterAddressTracker, and zooKeeper volatile, because they can be modified by a different thread, but are not exclusively accessed in a synchronized block (existing problem). New patch that fixes the problem, passes all tests. TestZookeeper seems to have good coverage. If I can think of more tests, I'll add them there.
        Lars Hofhansl made changes -
        Attachment 5682-all-v2.txt [ 12520813 ]
        Hide
        Lars Hofhansl added a comment -

        Upped to "critical". Without this the HBase client is pretty much useless in an AppServer setting where client can outlive the HBase cluster and ZK ensemble.
        (Testing within the Salesforce AppServer is how I noticed the problem initially.)

        Show
        Lars Hofhansl added a comment - Upped to "critical". Without this the HBase client is pretty much useless in an AppServer setting where client can outlive the HBase cluster and ZK ensemble. (Testing within the Salesforce AppServer is how I noticed the problem initially.)
        Lars Hofhansl made changes -
        Priority Major [ 3 ] Critical [ 2 ]
        Hide
        Lars Hofhansl added a comment -

        You think this should go into 0.92?

        Probably. I guess most folks have clients that they restart frequently, use thrift, or asynchhbase. But in its current form using the standard HBase client in an app server is very error prone if the HBase/ZK cluster is ever serviced without bringing the app server down in lock step.

        Didn't we add a check for if the connection is bad?

        Yeah with hbase-5153 but in 0.90 only. At some point we decided the fix there wasn't good and Ram patched it up for 0.90.
        This should subsime HBASE-5153. I'm happy to even put this in 0.90, but that's up to Ram.

        I'm interested in problems you see in hbase-5153 or issues you have w/ the implementation there that being the 0.96 client.

        What I saw in 0.96 is that the client was blocked for a very long time (gave up after a few minutes), even though I had set all timeouts to low values. This is also deadly in an app server setting. Might be a simple fix there, didn't dig deeper.

        Show
        Lars Hofhansl added a comment - You think this should go into 0.92? Probably. I guess most folks have clients that they restart frequently, use thrift, or asynchhbase. But in its current form using the standard HBase client in an app server is very error prone if the HBase/ZK cluster is ever serviced without bringing the app server down in lock step. Didn't we add a check for if the connection is bad? Yeah with hbase-5153 but in 0.90 only. At some point we decided the fix there wasn't good and Ram patched it up for 0.90. This should subsime HBASE-5153 . I'm happy to even put this in 0.90, but that's up to Ram. I'm interested in problems you see in hbase-5153 or issues you have w/ the implementation there that being the 0.96 client. What I saw in 0.96 is that the client was blocked for a very long time (gave up after a few minutes), even though I had set all timeouts to low values. This is also deadly in an app server setting. Might be a simple fix there, didn't dig deeper.
        Hide
        stack added a comment -

        On commit, change this '+ LOG.debug("Abort", t);' to include the passed in msg?

        Else, +1 on the patch. Let me ask N if he thinks TRUNK can pick up anything from this patch (maybe his keepalive should do this auto-reconnect but maybe it doesn't need it). What were you doing w/ it was taking a long time to recover?

        Show
        stack added a comment - On commit, change this '+ LOG.debug("Abort", t);' to include the passed in msg? Else, +1 on the patch. Let me ask N if he thinks TRUNK can pick up anything from this patch (maybe his keepalive should do this auto-reconnect but maybe it doesn't need it). What were you doing w/ it was taking a long time to recover?
        Hide
        Lars Hofhansl added a comment -

        Patch that removes the log statement Stack mentioned (had it in there for earlier debugging, forgot to remove it).

        Also adds a simple test with an HConnection that is created before the mini-cluster is started to prove that initialization is indeed lazy.
        (can't test with stopping and restarting the minicluster as new random ports are used each time).

        Show
        Lars Hofhansl added a comment - Patch that removes the log statement Stack mentioned (had it in there for earlier debugging, forgot to remove it). Also adds a simple test with an HConnection that is created before the mini-cluster is started to prove that initialization is indeed lazy. (can't test with stopping and restarting the minicluster as new random ports are used each time).
        Lars Hofhansl made changes -
        Attachment 5682-all-v3.txt [ 12520873 ]
        Hide
        Lars Hofhansl added a comment -

        all-v3 is what I like to commit tomorrow if there are no objections.

        Show
        Lars Hofhansl added a comment - all-v3 is what I like to commit tomorrow if there are no objections.
        Hide
        Jieshan Bean added a comment -

        Everything seems good to me. Only a minor doubt, is it necessary to close zooKeeper before set it as null?
        If HConnectionImplementation#managed is true, HConnectionImplementation#abort doesn't set closed to true, just calls close method. It makes sense to me. So the retry logic introduced in HBASE-5153 seems redundant.
        If one want to manage the connection by himself. If the connection is aborted. We should suggest to recreate the HConnection and HTable, right?

        Show
        Jieshan Bean added a comment - Everything seems good to me. Only a minor doubt, is it necessary to close zooKeeper before set it as null? If HConnectionImplementation#managed is true, HConnectionImplementation#abort doesn't set closed to true, just calls close method. It makes sense to me . So the retry logic introduced in HBASE-5153 seems redundant. If one want to manage the connection by himself. If the connection is aborted. We should suggest to recreate the HConnection and HTable, right?
        Hide
        Lars Hofhansl added a comment -

        Presumably close it not needed since the connection is known to be down in this case. To be save, I'll add that, and make sure it doesn't cause another hang.

        I think this is better than HBASE-5153, because it attempts to reconnect when the connection is needed and not when it was lost (in which case it is likely that the next retry will fail as well, leading to long hangs with no change for the caller to notice).

        Show
        Lars Hofhansl added a comment - Presumably close it not needed since the connection is known to be down in this case. To be save, I'll add that, and make sure it doesn't cause another hang. I think this is better than HBASE-5153 , because it attempts to reconnect when the connection is needed and not when it was lost (in which case it is likely that the next retry will fail as well, leading to long hangs with no change for the caller to notice).
        Hide
        Lars Hofhansl added a comment -

        Oh, and thanks for taking a look Jieshan

        Show
        Lars Hofhansl added a comment - Oh, and thanks for taking a look Jieshan
        Hide
        stack added a comment -

        @Nkeywal Hows' this relate to your TRUNK work (if at all)?

        Show
        stack added a comment - @Nkeywal Hows' this relate to your TRUNK work (if at all)?
        Hide
        Lars Hofhansl added a comment -

        One other strangeness I found is that none of ZKUtil methods actually throw exceptions. They retry (via RecoverableZooKeeper) and then just log a message if there is a failure. This is especially annoying with ZooKeeperWatcher, because there is no way of telling whether the connection succeeded of not from the outside.

        Show
        Lars Hofhansl added a comment - One other strangeness I found is that none of ZKUtil methods actually throw exceptions. They retry (via RecoverableZooKeeper) and then just log a message if there is a failure. This is especially annoying with ZooKeeperWatcher, because there is no way of telling whether the connection succeeded of not from the outside.
        Hide
        stack added a comment -

        Can we add an isAlive to ZKW?

        Show
        stack added a comment - Can we add an isAlive to ZKW?
        Hide
        Nicolas Liochon added a comment -

        .bq none of ZKUtil methods actually throw exceptions
        From what is see on 0.96 it should, as the return is not reached: the pattern is too call keeperException, and keeperException throws an exception.

          public void keeperException(KeeperException ke)
          throws KeeperException {
            LOG.error(prefix("Received unexpected KeeperException, re-throwing exception"), ke);
            throw ke;
          }
        
        Show
        Nicolas Liochon added a comment - .bq none of ZKUtil methods actually throw exceptions From what is see on 0.96 it should, as the return is not reached: the pattern is too call keeperException, and keeperException throws an exception. public void keeperException(KeeperException ke) throws KeeperException { LOG.error(prefix("Received unexpected KeeperException, re-throwing exception"), ke); throw ke; }
        Hide
        Lars Hofhansl added a comment -

        Yeah, my comment was wrong. It's not generally doing that.

        What I do find is if the ZK quorum is down, none of getZookeeperWatcher(), masterAddressTracker.start(), and rootRegionTracker.start() actually fail. They just retry and then happily return, which is as designed, because they are asynchronous.
        Would be nice to have a isAlive or waitForConnect method on ZKW that would throw if the connection could not be established.

        The attached patch is still a vast improvement, but it could be made better (even with zk timeout set to 100ms and retries to 3, it still take 22s for ensureZookeeperTrackers to finish).

        Show
        Lars Hofhansl added a comment - Yeah, my comment was wrong. It's not generally doing that. What I do find is if the ZK quorum is down, none of getZookeeperWatcher(), masterAddressTracker.start(), and rootRegionTracker.start() actually fail. They just retry and then happily return, which is as designed, because they are asynchronous. Would be nice to have a isAlive or waitForConnect method on ZKW that would throw if the connection could not be established. The attached patch is still a vast improvement, but it could be made better (even with zk timeout set to 100ms and retries to 3, it still take 22s for ensureZookeeperTrackers to finish).
        Hide
        Lars Hofhansl added a comment -

        Even isAlive or waitForConnect would need to rely on a timeout, so we wouldn't have won anything really.

        Show
        Lars Hofhansl added a comment - Even isAlive or waitForConnect would need to rely on a timeout, so we wouldn't have won anything really.
        Hide
        Lars Hofhansl added a comment -

        I think this is as good as we can get in 0.94.

        1. Removed the exception handling from ensureZookeeperTrackers none of these methods throw.
        2. added getZookeeperWatcher to two methods that just need a ZKW.

        The key is that an HConnection will never be left in a permanently useless state. Can file another jira for better timeouts.

        Show
        Lars Hofhansl added a comment - I think this is as good as we can get in 0.94. Removed the exception handling from ensureZookeeperTrackers none of these methods throw. added getZookeeperWatcher to two methods that just need a ZKW. The key is that an HConnection will never be left in a permanently useless state. Can file another jira for better timeouts.
        Lars Hofhansl made changes -
        Attachment 5682-all-v4.txt [ 12521017 ]
        Hide
        Nicolas Liochon added a comment -

        In 0.96 this should work, with the restriction that the logic is that you can get a non working connection, that will get fixed when you try to use it. It's a different mechanism than the one for HBaseAdmin, as HBaseAdmin first check the connection. Thz ZK mechanism is more efficient (you save a remote call to check that the connection is really working), but is more complex.

        However it seems it does not work at the end:

        What I saw in 0.96 is that the client was blocked for a very long time (gave up after a few minutes), even though I had set all timeouts to low values. This is also deadly in an app server setting. Might be a simple fix there, didn't dig deeper.

        @lars What did you exactly do? I can do the fix it on 0.96.

        Show
        Nicolas Liochon added a comment - In 0.96 this should work, with the restriction that the logic is that you can get a non working connection, that will get fixed when you try to use it. It's a different mechanism than the one for HBaseAdmin, as HBaseAdmin first check the connection. Thz ZK mechanism is more efficient (you save a remote call to check that the connection is really working), but is more complex. However it seems it does not work at the end: What I saw in 0.96 is that the client was blocked for a very long time (gave up after a few minutes), even though I had set all timeouts to low values. This is also deadly in an app server setting. Might be a simple fix there, didn't dig deeper. @lars What did you exactly do? I can do the fix it on 0.96.
        Hide
        Lars Hofhansl added a comment -

        Let me dig into 0.96 after I get this into 0.94... Wanna cut RC1 soon.

        From the past comments here I see no objections to posted patch... Will commit soon. Please speak up if you disagree.

        Show
        Lars Hofhansl added a comment - Let me dig into 0.96 after I get this into 0.94... Wanna cut RC1 soon. From the past comments here I see no objections to posted patch... Will commit soon. Please speak up if you disagree.
        Hide
        Lars Hofhansl added a comment -

        Committed to 0.94 only

        Show
        Lars Hofhansl added a comment - Committed to 0.94 only
        Lars Hofhansl made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Hadoop Flags Reviewed [ 10343 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #80 (See https://builds.apache.org/job/HBase-0.94/80/)
        HBASE-5682 Allow HConnectionImplementation to recover from ZK connection loss (Revision 1308596)

        Result = FAILURE
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #80 (See https://builds.apache.org/job/HBase-0.94/80/ ) HBASE-5682 Allow HConnectionImplementation to recover from ZK connection loss (Revision 1308596) Result = FAILURE larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #7 (See https://builds.apache.org/job/HBase-0.94-security/7/)
        HBASE-5682 Allow HConnectionImplementation to recover from ZK connection loss (Revision 1308596)

        Result = SUCCESS
        larsh :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
        • /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #7 (See https://builds.apache.org/job/HBase-0.94-security/7/ ) HBASE-5682 Allow HConnectionImplementation to recover from ZK connection loss (Revision 1308596) Result = SUCCESS larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java /hbase/branches/0.94/src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Lars
        See HBASE-6115. As we are not waiting for the root location to come up we get NPE now.

        Show
        ramkrishna.s.vasudevan added a comment - @Lars See HBASE-6115 . As we are not waiting for the root location to come up we get NPE now.
        Lars Hofhansl made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Lars Hofhansl
            Reporter:
            Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development