HBase
  1. HBase
  2. HBASE-10396

The constructor of HBaseAdmin may close the shared HConnection

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.94.16
    • Fix Version/s: None
    • Component/s: Admin, Client
    • Labels:
      None

      Description

      HBaseAdmin has the constructor:

        public HBaseAdmin(Configuration c)
        throws MasterNotRunningException, ZooKeeperConnectionException {
          this.conf = HBaseConfiguration.create(c);
          this.connection = HConnectionManager.getConnection(this.conf);
          ...
      

      As shown in above code, HBaseAdmin will get a cached HConnection or create a new HConnection and use this HConnection to connect to Master. Then, HBaseAdmin will delete the HConnection when connecting to master fail as follows:

          while ( true ){
            try {
              this.connection.getMaster();
              return;
            } catch (MasterNotRunningException mnre) {
              HConnectionManager.deleteStaleConnection(this.connection);
              this.connection = HConnectionManager.getConnection(this.conf);
            }
      

      The above code will invoke HConnectionManager#deleteStaleConnection to delete the HConnection from global HConnection cache. The risk is that the deleted HConnection might be sharing by other threads, such as HTable or HTablePool. Then, these threads which sharing the deleted HConnection will get closed HConnection exception:

      org.apache.hadoop.hbase.client.HConnectionManager$HConnectionImplementation@61bc59aa closed
      

      If users use HTablePool, the situation will become worse because closing HTable will only return HTable to HTablePool which won't reduce the reference count of the closed HConnection. Then, the closed HConnection will always be used before clearing HTablePool. In 0.94, some modules such as Rest server are using HTablePool, therefore may suffer from this problem.

      1. HBASE-10396-0.94-v1.patch
        1 kB
        cuijianwei
      2. HBASE-10396-0.94-v2.patch
        3 kB
        cuijianwei

        Activity

        Hide
        cuijianwei added a comment -

        To alleviate this problem, we might invoke HConnection#close when connecting to Master fail. However, this won't close the HConnection immediately if the HConnection is sharing by other threads. Then, the same HConnection will be got in the next retry.

        Show
        cuijianwei added a comment - To alleviate this problem, we might invoke HConnection#close when connecting to Master fail. However, this won't close the HConnection immediately if the HConnection is sharing by other threads. Then, the same HConnection will be got in the next retry.
        Hide
        chunhui shen added a comment -

        Trunk seems fixed this problem. Make a backport?

        Show
        chunhui shen added a comment - Trunk seems fixed this problem. Make a backport?
        Hide
        cuijianwei added a comment -

        Thanks for your comment chunhui shen, I go through the code of HBaseAdmin in trunk. The HConnection will be closed in HBaseAdmin#close so that fixed the problem. The code of HBaseAdmin changed a lot between 0.94 and trunk, will we supply a patch for 0.94 to fix this problem?

        Show
        cuijianwei added a comment - Thanks for your comment chunhui shen , I go through the code of HBaseAdmin in trunk. The HConnection will be closed in HBaseAdmin#close so that fixed the problem. The code of HBaseAdmin changed a lot between 0.94 and trunk, will we supply a patch for 0.94 to fix this problem?
        Hide
        stack added a comment -

        cuijianwei Go for it boss.

        Show
        stack added a comment - cuijianwei Go for it boss.
        Hide
        cuijianwei added a comment -

        will not delete as stale HConnection when connecting master fail

        Show
        cuijianwei added a comment - will not delete as stale HConnection when connecting master fail
        Hide
        cuijianwei added a comment -

        stack Thanks for your concern. I make a patch for 0.94 to use HConnectionManager#deleteConnection(Configuration) so that will not delete the shared HConnection if other threads are using it. BTW, There are two constructors of HBaseAdmin:

          public HBaseAdmin(Configuration c) throws MasterNotRunningException, ZooKeeperConnectionException;
        

        and:

        public HBaseAdmin(HConnection connection) throws MasterNotRunningException, ZooKeeperConnectionException;
        

        The first constructor will retry when invoking HConnection#getMaster fail, which performing differently as the second constructor. The second constructor will not retry if connecting master fail. As HConnection#getMaster will also retry if connecting fail, is it reasonable that the first constructor will not retry when invoking HConnection#getMaster fail? Then, the two constructors will have the same action.

        Show
        cuijianwei added a comment - stack Thanks for your concern. I make a patch for 0.94 to use HConnectionManager#deleteConnection(Configuration) so that will not delete the shared HConnection if other threads are using it. BTW, There are two constructors of HBaseAdmin: public HBaseAdmin(Configuration c) throws MasterNotRunningException, ZooKeeperConnectionException; and: public HBaseAdmin(HConnection connection) throws MasterNotRunningException, ZooKeeperConnectionException; The first constructor will retry when invoking HConnection#getMaster fail, which performing differently as the second constructor. The second constructor will not retry if connecting master fail. As HConnection#getMaster will also retry if connecting fail, is it reasonable that the first constructor will not retry when invoking HConnection#getMaster fail? Then, the two constructors will have the same action.
        Hide
        chunhui shen added a comment -

        After the patch,

        catch (MasterNotRunningException mnre) {
                HConnectionManager.deleteConnection(this.connection);
                this.connection = HConnectionManager.getConnection(this.conf);
              }
        

        It means the connection won't be changed in the while block since always get the cached one. Is it expected?

        I think we could move the HBaseAdmin code from trunk to 0.94

        Show
        chunhui shen added a comment - After the patch, catch (MasterNotRunningException mnre) { HConnectionManager.deleteConnection( this .connection); this .connection = HConnectionManager.getConnection( this .conf); } It means the connection won't be changed in the while block since always get the cached one. Is it expected? I think we could move the HBaseAdmin code from trunk to 0.94
        Hide
        cuijianwei added a comment -

        Backport trunk code for constructors of HBaseAdmin

        Show
        cuijianwei added a comment - Backport trunk code for constructors of HBaseAdmin
        Hide
        cuijianwei added a comment -

        Thanks for your advice chunhui shen, I think moving trunk code of HBaseAdmin to 0.94 is a better choice. In patch 0.94-v1, HBaseAdmin might get a new HConnection only when all the other threads deleted the shared HConnection, which is not good enough. The trunk code of HBaseAdmin is:

        public HBaseAdmin(Configuration c)
          throws MasterNotRunningException, ZooKeeperConnectionException, IOException {
            // Will not leak connections, as the new implementation of the constructor
            // does not throw exceptions anymore.
            this(HConnectionManager.getConnection(new Configuration(c)));
            this.cleanupConnectionOnClose = true;
          }
        
        public HBaseAdmin(HConnection connection)
          throws MasterNotRunningException, ZooKeeperConnectionException {
            ....
          }
        

        The constructs of HBaseAdmin in trunk will not throw exceptions. However, in 0.94, HConnection#getMaster will throw exceptions in constructor which might cause Hconnection leak. Therefore, the second patch will not invoke HConnection#getMaster in HBaseAdmin. Then, HConnection#getMaster will be invoked when really needing to connect to master.

        Show
        cuijianwei added a comment - Thanks for your advice chunhui shen , I think moving trunk code of HBaseAdmin to 0.94 is a better choice. In patch 0.94-v1, HBaseAdmin might get a new HConnection only when all the other threads deleted the shared HConnection, which is not good enough. The trunk code of HBaseAdmin is: public HBaseAdmin(Configuration c) throws MasterNotRunningException, ZooKeeperConnectionException, IOException { // Will not leak connections, as the new implementation of the constructor // does not throw exceptions anymore. this (HConnectionManager.getConnection( new Configuration(c))); this .cleanupConnectionOnClose = true ; } public HBaseAdmin(HConnection connection) throws MasterNotRunningException, ZooKeeperConnectionException { .... } The constructs of HBaseAdmin in trunk will not throw exceptions. However, in 0.94, HConnection#getMaster will throw exceptions in constructor which might cause Hconnection leak. Therefore, the second patch will not invoke HConnection#getMaster in HBaseAdmin. Then, HConnection#getMaster will be invoked when really needing to connect to master.

          People

          • Assignee:
            Unassigned
            Reporter:
            cuijianwei
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development