HBase
  1. HBase
  2. HBASE-2669

HCM.shutdownHook causes data loss with hbase.client.write.buffer != 0

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.90.0
    • Component/s: Client
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      data loss

      Description

      In my application I set hbase.client.write.buffer to a reasonably small value (roughly 64 edits) in order to try to batch a few Put together before talking to HBase. When my application does a graceful shutdown, I call HTable#flushCommits in order to flush any pending change to HBase. I want to do the same thing when I get a SIGTERM by using Runtime#addShutdownHook but this is impossible since HConnectionManager already registers a shutdown hook that invokes HConnectionManager#deleteAllConnections. This static method closes all the connections to HBase and then all connections to ZooKeeper. Because all shutdown hooks run in parallel, my hook will attempt to flush edits while connections are getting closed.

      There is no way to guarantee the order in which the hooks will execute, so I propose that we remove the hook in the HCM altogether and provide some user-visible API they call in their own hook after they're done flushing their stuff, if they really want to do a graceful shutdown. I expect that a lot of users won't use a hook though, otherwise this issue would have cropped up already. For those users, connections won't get "gracefully" terminated, but I don't think that would be a problem since the underlying TCP socket will get closed by the OS anyway, so things like ZooKeeper and such should realize that the connection has been terminated and assume the client is gone, and do the necessary clean-up on their side.

      An alternate fix would be to leave the hook in place by default but keep a reference to it and add a user-visible API to be able to un-register the hook. I find this ugly.

      Thoughts?

      1. 2669-v2.txt
        24 kB
        stack
      2. 2669.txt
        4 kB
        stack

        Issue Links

          Activity

          stack made changes -
          Resolution Fixed [ 1 ]
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Hide
          stack added a comment -

          Committed. Thanks for the review Jon. Did all you suggested on commit (Did not add back your RIT logging – you can do taht if you need it).

          Show
          stack added a comment - Committed. Thanks for the review Jon. Did all you suggested on commit (Did not add back your RIT logging – you can do taht if you need it).
          stack made changes -
          Attachment 2669-v2.txt [ 12457494 ]
          Hide
          stack added a comment -

          Version 2. In this version, we add more explicit closeups of HConnection and then add a bunch of javadoc explaining how HConnection works and how its cleanup is done.

          A set of changes that allow doing away with shutdown hook in client.
          
          M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
            Removed unused import and changed message from info to debug
            -- when info it shows in shell whenever we run a command.
          M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
            Changed message from info to debug -- when info it shows in shell
            whenever we run a command.
          M src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
            Changed order; online region before we tell everyone where the
            region is (I changed this order recently but reviewing comments
            and issues I can't figure why I did it -- I think there was a
            reason but can't recall so just put this back until we trip
            over the issue again.  My change made it so that we had
            strange issue where we'd get a NSRE though the region was coming
            up here on this server... rather than do retries of NSREs,
            put it into online servers before updating zk... again).
          M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
            Removed noisy message
          M src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java
            Have this interface implement Stoppable... Some implemenations
            need their Stop called.
          M src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java
            Startup wont work w/o this change.
          M src/main/java/org/apache/hadoop/hbase/master/LogCleaner.java
            Wrap the chore run so we can call the stop on all log cleaners.
          M src/main/java/org/apache/hadoop/hbase/master/TimeToLiveLogCleaner.java
          M src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java
            Implement Stoppable
          M src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java
            Cleanup all connections on way out.
          M src/main/java/org/apache/hadoop/hbase/util/HMerge.java
            Cleanup proxies.... was false.
          M src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
          M src/main/java/org/apache/hadoop/hbase/client/HConnection.java
            Javadoc explaining how HConnections work.
          M src/main/java/org/apache/hadoop/hbase/client/HTablePool.java
            Make it so we make a new Configuration and that we then
            do our own cleanup when pool is shutdown.
          M src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
            Use alternate method now the one that takes a Connection removed.
          M src/main/java/org/apache/hadoop/hbase/client/HTable.java
            More javadoc on how HConnection works.
          
          Show
          stack added a comment - Version 2. In this version, we add more explicit closeups of HConnection and then add a bunch of javadoc explaining how HConnection works and how its cleanup is done. A set of changes that allow doing away with shutdown hook in client. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java Removed unused import and changed message from info to debug -- when info it shows in shell whenever we run a command. M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java Changed message from info to debug -- when info it shows in shell whenever we run a command. M src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java Changed order; online region before we tell everyone where the region is (I changed this order recently but reviewing comments and issues I can't figure why I did it -- I think there was a reason but can't recall so just put this back until we trip over the issue again. My change made it so that we had strange issue where we'd get a NSRE though the region was coming up here on this server... rather than do retries of NSREs, put it into online servers before updating zk... again). M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java Removed noisy message M src/main/java/org/apache/hadoop/hbase/master/LogCleanerDelegate.java Have this interface implement Stoppable... Some implemenations need their Stop called. M src/main/java/org/apache/hadoop/hbase/master/HMasterCommandLine.java Startup wont work w/o this change. M src/main/java/org/apache/hadoop/hbase/master/LogCleaner.java Wrap the chore run so we can call the stop on all log cleaners. M src/main/java/org/apache/hadoop/hbase/master/TimeToLiveLogCleaner.java M src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java Implement Stoppable M src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java Cleanup all connections on way out. M src/main/java/org/apache/hadoop/hbase/util/HMerge.java Cleanup proxies.... was false . M src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java M src/main/java/org/apache/hadoop/hbase/client/HConnection.java Javadoc explaining how HConnections work. M src/main/java/org/apache/hadoop/hbase/client/HTablePool.java Make it so we make a new Configuration and that we then do our own cleanup when pool is shutdown. M src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java Use alternate method now the one that takes a Connection removed. M src/main/java/org/apache/hadoop/hbase/client/HTable.java More javadoc on how HConnection works.
          stack made changes -
          Assignee Benoit Sigoure [ tsuna ] stack [ stack ]
          Hide
          stack added a comment -

          Let me fix up this patch and make it apply to trunk. I think its general drift is fine. Whats missing now is a bunch of explaination of how Connections work and are shared – of how the sharing is keyed by Configuration and of how if you want a clean shutdown of your tables, then you will need to do the ugly HConnectionManager.deleteConnection stuff for now, in 0.90, at least.

          Running tests.

          Show
          stack added a comment - Let me fix up this patch and make it apply to trunk. I think its general drift is fine. Whats missing now is a bunch of explaination of how Connections work and are shared – of how the sharing is keyed by Configuration and of how if you want a clean shutdown of your tables, then you will need to do the ugly HConnectionManager.deleteConnection stuff for now, in 0.90, at least. Running tests.
          stack made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          stack made changes -
          Link This issue is related to HBASE-2952 [ HBASE-2952 ]
          Hide
          Jean-Daniel Cryans added a comment -

          You want to commit this Stack?

          Show
          Jean-Daniel Cryans added a comment - You want to commit this Stack?
          Hide
          Benoit Sigoure added a comment -

          I'm OK with your patch stack, thanks for putting it together.

          Minor stylistic nit: it's not clear to me why the import of MetaScanner.MetaScannerVisitor has moved around in HMaster.java, lines are now out of order. Plus, I don't think HMaster.java should be changed as part of this issue, as this issue has nothing to do with the master.

          Show
          Benoit Sigoure added a comment - I'm OK with your patch stack, thanks for putting it together. Minor stylistic nit: it's not clear to me why the import of MetaScanner.MetaScannerVisitor has moved around in HMaster.java , lines are now out of order. Plus, I don't think HMaster.java should be changed as part of this issue, as this issue has nothing to do with the master.
          stack made changes -
          Attachment 2669.txt [ 12450726 ]
          Hide
          stack added a comment -

          How about this Beniôt? It removes the shutdown hook and then instead inside in TOF, on close, it does explicit HConnectionManager.deleteAllConnections(true); I haven't tried it yet. If you think it way to go, I'll try it doing MR to see if it beaks anything.

          Show
          stack added a comment - How about this Beniôt? It removes the shutdown hook and then instead inside in TOF, on close, it does explicit HConnectionManager.deleteAllConnections(true); I haven't tried it yet. If you think it way to go, I'll try it doing MR to see if it beaks anything.
          Hide
          stack added a comment -

          Nice trick Héctor (/me puts it in backpocket)

          HBASE-1999 added the shutdown hook to get around noise in zk logs about expired sessions during the running of big MR jobs. I thought it was needed for unit tests but I see that tests explicitly call HCM#deleteAllConnections when shutting down minicluster. Running patch with the shutdown hook removed seems fine.

          Show
          stack added a comment - Nice trick Héctor (/me puts it in backpocket) HBASE-1999 added the shutdown hook to get around noise in zk logs about expired sessions during the running of big MR jobs. I thought it was needed for unit tests but I see that tests explicitly call HCM#deleteAllConnections when shutting down minicluster. Running patch with the shutdown hook removed seems fine.
          Hide
          Héctor Izquierdo added a comment -

          An ugly workaround:

          private Thread getEvilHTableShutdownHook() throws Exception {

          Class clazz = Class.forName("java.lang.ApplicationShutdownHooks");

          Field[] fields = clazz.getDeclaredFields();

          for (Field field : fields) {

          if (field.getType() == IdentityHashMap.class) {
          field.setAccessible(true);
          IdentityHashMap<Thread, Thread> hooks = (IdentityHashMap<Thread, Thread>) field.get(null);

          for (Map.Entry<Thread, Thread> entries : hooks.entrySet()) {
          System.out.println(entries.getKey().getName());
          if (entries.getValue().getName().equals("HCM.shutdownHook"))

          { return entries.getValue(); }

          }

          }
          }

          return null;
          }

          Show
          Héctor Izquierdo added a comment - An ugly workaround: private Thread getEvilHTableShutdownHook() throws Exception { Class clazz = Class.forName("java.lang.ApplicationShutdownHooks"); Field[] fields = clazz.getDeclaredFields(); for (Field field : fields) { if (field.getType() == IdentityHashMap.class) { field.setAccessible(true); IdentityHashMap<Thread, Thread> hooks = (IdentityHashMap<Thread, Thread>) field.get(null); for (Map.Entry<Thread, Thread> entries : hooks.entrySet()) { System.out.println(entries.getKey().getName()); if (entries.getValue().getName().equals("HCM.shutdownHook")) { return entries.getValue(); } } } } return null; }
          Hide
          Benoit Sigoure added a comment -

          As you wrote Stack, there's no easy way to get to the HCM's hook. First of all, the HCM itself doesn't retain a reference to it, so right now it's just impossible to reach. Even if a reference was retained, the suppressHdfsShutdownHook hack in HRegionServer seems too ugly and fragile to me. We don't need to expose this internal implementation detail of the HCM to user. Instead we can just have a static method in HTable that the user can call to perform a graceful shutdown.

          But I also doubt this hook is useful at all. As I said, I'm not sure we need to properly close the connections ourselves. The OS will take care of them anyway, and whatever the client was talking to will be notified that the socket was closed on the other side. Maybe we can just remove the hook altogether and get away with it.

          BTW, sorry I didn't mean to close 0.20.5 with this issue, I'm working with trunk so this has nothing to do with 0.20.5 anyway. I also noticed that suppressHdfsShutdownHook has gone away in trunk. Not sure why, but that's a good thing.

          Show
          Benoit Sigoure added a comment - As you wrote Stack, there's no easy way to get to the HCM's hook. First of all, the HCM itself doesn't retain a reference to it, so right now it's just impossible to reach. Even if a reference was retained, the suppressHdfsShutdownHook hack in HRegionServer seems too ugly and fragile to me. We don't need to expose this internal implementation detail of the HCM to user. Instead we can just have a static method in HTable that the user can call to perform a graceful shutdown. But I also doubt this hook is useful at all. As I said, I'm not sure we need to properly close the connections ourselves. The OS will take care of them anyway, and whatever the client was talking to will be notified that the socket was closed on the other side. Maybe we can just remove the hook altogether and get away with it. BTW, sorry I didn't mean to close 0.20.5 with this issue, I'm working with trunk so this has nothing to do with 0.20.5 anyway. I also noticed that suppressHdfsShutdownHook has gone away in trunk. Not sure why, but that's a good thing.
          Hide
          stack added a comment -

          This shutdown hook registration is buried way deap. Its way down in HCM and its registered on static initialization. You can't really get an HCM legitimately, not w/o doing some instanceof and casting of an HConnection returned when you do a getConnection. What would you suggest Benoit? The HRS#suppressHdfsShutdownHook looks pretty good to me compared to exposing HCM and letting user get at the shutdown hook that way?

          Show
          stack added a comment - This shutdown hook registration is buried way deap. Its way down in HCM and its registered on static initialization. You can't really get an HCM legitimately, not w/o doing some instanceof and casting of an HConnection returned when you do a getConnection. What would you suggest Benoit? The HRS#suppressHdfsShutdownHook looks pretty good to me compared to exposing HCM and letting user get at the shutdown hook that way?
          stack made changes -
          Field Original Value New Value
          Fix Version/s 0.21.0 [ 12313607 ]
          Fix Version/s 0.20.5 [ 12314800 ]
          Hide
          stack added a comment -

          Moving out of 0.20.5. There is a workaround for now. See suppressHdfsShutdownHook in HRegionServer for how to remove the installed HCM shutdown hook and install your own instead... running the HCM one after yours. Its ugly but it works. While the implication is dataloss if you SIGTERM the hosting application, this is sort of a new request/feature, or putting it another way, its a problem we've had for a long time, not particular to 0.20.5. I don't thnk it a blocker. Sink the coming RC if you think otherwise Benoit by voting against it but for now, I'd like to show there is movement around 0.20.5 release by posting a new RC.

          Show
          stack added a comment - Moving out of 0.20.5. There is a workaround for now. See suppressHdfsShutdownHook in HRegionServer for how to remove the installed HCM shutdown hook and install your own instead... running the HCM one after yours. Its ugly but it works. While the implication is dataloss if you SIGTERM the hosting application, this is sort of a new request/feature, or putting it another way, its a problem we've had for a long time, not particular to 0.20.5. I don't thnk it a blocker. Sink the coming RC if you think otherwise Benoit by voting against it but for now, I'd like to show there is movement around 0.20.5 release by posting a new RC.
          Hide
          stack added a comment -

          hmm... well yeah, its pretty critical prob. want to work on this today then?

          Show
          stack added a comment - hmm... well yeah, its pretty critical prob. want to work on this today then?
          Hide
          stack added a comment -

          In old days, we had similar prob w/ hdfs. We wanted to run a shutdown cleanup of hbase hook but hdfs would be running its clean up at same time and we couldn't guarantee order.

          Using reflection, we looked for hdsf hook, if present, unregistered it but kept a reference and then in our shutdown hook, after was done, we'd call the hdfs one. Lets fix this benoit. Mind if I move it out of 0.20.5 though? Its a prob. but not end of world and I'd like to get a 0.20.5 rolled today. Thanks.

          Show
          stack added a comment - In old days, we had similar prob w/ hdfs. We wanted to run a shutdown cleanup of hbase hook but hdfs would be running its clean up at same time and we couldn't guarantee order. Using reflection, we looked for hdsf hook, if present, unregistered it but kept a reference and then in our shutdown hook, after was done, we'd call the hdfs one. Lets fix this benoit. Mind if I move it out of 0.20.5 though? Its a prob. but not end of world and I'd like to get a 0.20.5 rolled today. Thanks.
          Benoit Sigoure created issue -

            People

            • Assignee:
              stack
              Reporter:
              Benoit Sigoure
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development