HBase
  1. HBase
  2. HBASE-2925

LRU of HConnectionManager.HBASE_INSTANCES breaks if HBaseConfiguration is changed

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.20.3, 0.90.0
    • Fix Version/s: 0.90.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      noob

      Description

      HConnectionManager.getConnection(config) caches the created TableServer in HBASE_INSTANCES (a LinkedHashMap ) which is keyed by the configuration instance itself.
      Given the current implementation of hashCode() (and equals()) of HBaseConfiguration, the hash code of the configuration is changed if any of its properties are changed, which will cause the keys of HBASE_INSTANCES to be inconsistent with the hashtable that contains them, making some entries unreachable.
      In this case, when the map's LRU strategy needs to remove the oldest entry, it tries to remove it based on the oldest key, which no longer gives the original hash code, therefore the lookup in HBASE_INSTANCES.remove(oldest) doesn't actually remove anything.

      This has been observed to lead to OOM errors in long running clients.

      1. 2925-v2.txt
        20 kB
        stack
      2. 2925.txt
        17 kB
        stack
      3. SimpleHConnectionManagerLeakReplicator.java
        3 kB
        Robert Mahfoud

        Issue Links

          Activity

          Hide
          Robert Mahfoud added a comment -

          This is a class that I put together that illustrates this problem and replicate it. Could be used as a basis for the unit test for a patch.

          Show
          Robert Mahfoud added a comment - This is a class that I put together that illustrates this problem and replicate it. Could be used as a basis for the unit test for a patch.
          Hide
          Robert Mahfoud added a comment -

          The only fix I could think of is to clone the configuration before inserting in the new TableServer in HBASE_INSTANCES. This way, LRU will work since the key cannot be changed since no other object holds a reference to it.
          This will add the overhead of creating a new HBaseConfiguration instance with every connection, which is minimal if the cache works as it should.

          Show
          Robert Mahfoud added a comment - The only fix I could think of is to clone the configuration before inserting in the new TableServer in HBASE_INSTANCES . This way, LRU will work since the key cannot be changed since no other object holds a reference to it. This will add the overhead of creating a new HBaseConfiguration instance with every connection, which is minimal if the cache works as it should.
          Hide
          stack added a comment -

          @Robert What if we just removed the dump hash and equals and used object equality instead? I think it a bit much trying to equate Configuration objects; i.e. they are the same if they have "same" config where "same" is every properties's hash equates. If we were to go the route of trying to equate Configurations by the properties they carry, we should equate the values 'true', 'True', 'TRUE', and if a boolean !0 (anything but zero).

          Thanks for filing this issue.

          Show
          stack added a comment - @Robert What if we just removed the dump hash and equals and used object equality instead? I think it a bit much trying to equate Configuration objects; i.e. they are the same if they have "same" config where "same" is every properties's hash equates. If we were to go the route of trying to equate Configurations by the properties they carry, we should equate the values 'true', 'True', 'TRUE', and if a boolean !0 (anything but zero). Thanks for filing this issue.
          Hide
          stack added a comment -

          Hmm... Looks like I thought removing hashcode from HBC a good idea a while back too (See comment in HBASE-1976).

          Show
          stack added a comment - Hmm... Looks like I thought removing hashcode from HBC a good idea a while back too (See comment in HBASE-1976 ).
          Hide
          stack added a comment -

          Here's a start. Its not done yet but shows direction: i.e. removing hashcode and equals from HBC. TODO, is test that prove that HBASE-1251 is still fixed (I think thing to do here is just read configs down in TableServers out of the conf each time rather than once up front so if number of retries is changed mid-use, subsequent invocations will pick up new config. – let me see).

          Removes hashcode and equals from HBC.  Use Object hashcode instead
          of try and have Configurations with same exact content somehow
          equate.
          
          M a/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java
            Removed hashCode and equals.
          M a/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
          M a/src/main/java/org/apache/hadoop/hbase/util/HMerge.java
          M a/src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java
          M a/src/test/java/org/apache/hadoop/hbase/util/DisabledTestMetaUtils.java
            Renamed deleteConnectionInfo as deleteConnection.
          M a/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
            Fixed up some comments and javadoc.  Moved a few methods around.
            Use Configuration instance itself as key for TableServer instances
            rather than the Configuation hash code int.
            Renamed deleteConnectionInfo as deleteConnection.
          M a/src/main/java/org/apache/hadoop/hbase/client/HTable.java
            Fixed up javadoc trying to explain implication of not passing
            Configuration constructing an HTable instance.
          M a/src/main/java/org/apache/hadoop/hbase/client/HTableFactory.java
            White space removal.
          M a/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java
            Added testNewConnectionsDoesNotOOME.  It calls slightly
            refactored Robert Mahfoud illustrative code.  Added assertions.
            Made it not run for ever.
          
          Show
          stack added a comment - Here's a start. Its not done yet but shows direction: i.e. removing hashcode and equals from HBC. TODO, is test that prove that HBASE-1251 is still fixed (I think thing to do here is just read configs down in TableServers out of the conf each time rather than once up front so if number of retries is changed mid-use, subsequent invocations will pick up new config. – let me see). Removes hashcode and equals from HBC. Use Object hashcode instead of try and have Configurations with same exact content somehow equate. M a/src/main/java/org/apache/hadoop/hbase/HBaseConfiguration.java Removed hashCode and equals. M a/src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java M a/src/main/java/org/apache/hadoop/hbase/util/HMerge.java M a/src/test/java/org/apache/hadoop/hbase/HBaseClusterTestCase.java M a/src/test/java/org/apache/hadoop/hbase/util/DisabledTestMetaUtils.java Renamed deleteConnectionInfo as deleteConnection. M a/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java Fixed up some comments and javadoc. Moved a few methods around. Use Configuration instance itself as key for TableServer instances rather than the Configuation hash code int . Renamed deleteConnectionInfo as deleteConnection. M a/src/main/java/org/apache/hadoop/hbase/client/HTable.java Fixed up javadoc trying to explain implication of not passing Configuration constructing an HTable instance. M a/src/main/java/org/apache/hadoop/hbase/client/HTableFactory.java White space removal. M a/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java Added testNewConnectionsDoesNotOOME. It calls slightly refactored Robert Mahfoud illustrative code. Added assertions. Made it not run for ever.
          Hide
          stack added a comment -

          Oh Robert, I just noticed that you did not grant Apache license on your test code. If intentional, np, I'll back your test out of my patch. Just say so.

          Show
          stack added a comment - Oh Robert, I just noticed that you did not grant Apache license on your test code. If intentional, np, I'll back your test out of my patch. Just say so.
          Hide
          Robert Mahfoud added a comment -

          @stack, thank you for picking up this issue. Feel free to reuse the code provided in SimpleHConnectionManagerLeakReplicator.java in any way you want. (Let me know if this is not enough I could submit another file with Apache license added.)

          Back to the discussion, I agree that removing the hasCode() and equals() methods does resolve this issue. However, I wonder if it defeats the purpose of having a connection cache from the first place. You also alluded to the problem of the configuration settings changing after the connection is open, which, if we rely solely on Object equality, could lead to subtle error or unexpected behavior.

          Hence my suggestion to pin down the configuration once the TableServers instance is created. The disadvantage there is that small/unrelated changes in the configuration will invalidate the cache entry as well.

          If that is deemed a real problem given the common use cases, I was thinking maybe we define "equality" of the configuration for the purposes of caching as the equality of all properties that start with hbase. or zk. or any other prefix whose properties could be used as possible connection parameters. Not sure if the comparison becomes too slow in that case, but what is more important is whether correctness is compromised if we make the assumption that only properties starting with those prefixes affect the connection.

          Show
          Robert Mahfoud added a comment - @stack, thank you for picking up this issue. Feel free to reuse the code provided in SimpleHConnectionManagerLeakReplicator.java in any way you want. (Let me know if this is not enough I could submit another file with Apache license added.) Back to the discussion, I agree that removing the hasCode() and equals() methods does resolve this issue. However, I wonder if it defeats the purpose of having a connection cache from the first place. You also alluded to the problem of the configuration settings changing after the connection is open, which, if we rely solely on Object equality, could lead to subtle error or unexpected behavior. Hence my suggestion to pin down the configuration once the TableServers instance is created. The disadvantage there is that small/unrelated changes in the configuration will invalidate the cache entry as well. If that is deemed a real problem given the common use cases, I was thinking maybe we define "equality" of the configuration for the purposes of caching as the equality of all properties that start with hbase. or zk. or any other prefix whose properties could be used as possible connection parameters. Not sure if the comparison becomes too slow in that case, but what is more important is whether correctness is compromised if we make the assumption that only properties starting with those prefixes affect the connection.
          Hide
          stack added a comment -

          @Robert ...I wonder if it defeats the purpose of having a connection cache from the first place.

          As I see it, the main benefit to the cache is saving on region lookups, setup of zk connection, and master proxy setup. Having the likes of the following config cached is secondary: e.g.

              private final long pause;
              private final int numRetries;
              private final int maxRPCAttempts;
              private final long rpcTimeout;
              private final int prefetchRegionLimit;
          

          In fact, I'm now thinking that if a user changes any of the above in a Configuration that is being used as a key in HCM#HBASE_INSTANCES, that its ok if we don't notice the change as long as we doc the fact that the Configuration is read on instantiation of the HTable and then no more. I don't tthink this should 'surprise' the user too much and they can just go create new HTable with the new Configuration if they really want their new config. to take hold (Making things work this way is similar to what you suggest only you would copy the Configuration before adding it as a key).

          As to your last suggestion, I think we should move away from trying to equate Configurations at all; there be daemons that way.

          Let me know what you think. If you are agreeable, I'll work up the patch some more mostly adding doc clarifying what we've agreed here.

          Show
          stack added a comment - @Robert ...I wonder if it defeats the purpose of having a connection cache from the first place. As I see it, the main benefit to the cache is saving on region lookups, setup of zk connection, and master proxy setup. Having the likes of the following config cached is secondary: e.g. private final long pause; private final int numRetries; private final int maxRPCAttempts; private final long rpcTimeout; private final int prefetchRegionLimit; In fact, I'm now thinking that if a user changes any of the above in a Configuration that is being used as a key in HCM#HBASE_INSTANCES, that its ok if we don't notice the change as long as we doc the fact that the Configuration is read on instantiation of the HTable and then no more. I don't tthink this should 'surprise' the user too much and they can just go create new HTable with the new Configuration if they really want their new config. to take hold (Making things work this way is similar to what you suggest only you would copy the Configuration before adding it as a key). As to your last suggestion, I think we should move away from trying to equate Configurations at all; there be daemons that way. Let me know what you think. If you are agreeable, I'll work up the patch some more mostly adding doc clarifying what we've agreed here.
          Hide
          Robert Mahfoud added a comment -

          Making things work this way is similar to what you suggest only you would copy the Configuration before adding it as a key ...

          This is what I had in mind... Sorry if I didn't make it clear in previous comments.

          ... its ok if we don't notice the change as long as we doc the fact that the Configuration is read on instantiation of the HTable and then no more ...

          I agree that these semantics are good for all the use cases I could think of. So feel free to proceed with your patch.

          Show
          Robert Mahfoud added a comment - Making things work this way is similar to what you suggest only you would copy the Configuration before adding it as a key ... This is what I had in mind... Sorry if I didn't make it clear in previous comments. ... its ok if we don't notice the change as long as we doc the fact that the Configuration is read on instantiation of the HTable and then no more ... I agree that these semantics are good for all the use cases I could think of. So feel free to proceed with your patch.
          Hide
          stack added a comment -

          This version of the patch just adds javadoc to HTable explaining the advantage of shared Configuration – the sharing of zookeeper connection, cache of region locations, etc.

          If you have a minute, give it a gander Robert and if its good w/ you, I'll go ahead and commit.

          Show
          stack added a comment - This version of the patch just adds javadoc to HTable explaining the advantage of shared Configuration – the sharing of zookeeper connection, cache of region locations, etc. If you have a minute, give it a gander Robert and if its good w/ you, I'll go ahead and commit.
          Hide
          Robert Mahfoud added a comment -

          You may have left out a couple of print statements in TestHCM. Not sure if that was intentional.

                System.out.println("Hash Code: " + configuration.hashCode());
                ...
                    System.out.println("!! Got same connection for once !!");
          

          Otherwise it's good to go.

          Show
          Robert Mahfoud added a comment - You may have left out a couple of print statements in TestHCM. Not sure if that was intentional. System .out.println( "Hash Code: " + configuration.hashCode()); ... System .out.println( "!! Got same connection for once !!" ); Otherwise it's good to go.
          Hide
          stack added a comment -

          @Robert Yeah I removed some, left others but also added asserts so tests should fail is regression. Thanks for doing up the test. I just committed v2.

          Show
          stack added a comment - @Robert Yeah I removed some, left others but also added asserts so tests should fail is regression. Thanks for doing up the test. I just committed v2.

            People

            • Assignee:
              Unassigned
              Reporter:
              Robert Mahfoud
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development