HBase
  1. HBase
  2. HBASE-10263

make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.98.0, 0.99.0
    • Component/s: io
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      4 new configurations are introduced:
      hbase.lru.blockcache.single.percentage / hbase.lru.blockcache.multi.percentage / hbase.lru.blockcache.memory.percentage / hbase.lru.rs.inmemoryforcemode
      the first 3 enable user to config different single/multi/memory ratio in block cache; the last one determines if preemptive mode for memory block is enabled
      Show
      4 new configurations are introduced: hbase.lru.blockcache.single.percentage / hbase.lru.blockcache.multi.percentage / hbase.lru.blockcache.memory.percentage / hbase.lru.rs.inmemoryforcemode the first 3 enable user to config different single/multi/memory ratio in block cache; the last one determines if preemptive mode for memory block is enabled

      Description

      currently the single/multi/in-memory ratio in LruBlockCache is hardcoded 1:2:1, which can lead to somewhat counter-intuition behavior for some user scenario where in-memory table's read performance is much worse than ordinary table when two tables' data size is almost equal and larger than regionserver's cache size (we ever did some such experiment and verified that in-memory table random read performance is two times worse than ordinary table).

      this patch fixes above issue and provides:
      1. make single/multi/in-memory ratio user-configurable
      2. provide a configurable switch which can make in-memory block preemptive, by preemptive means when this switch is on in-memory block can kick out any ordinary block to make room until no ordinary block, when this switch is off (by default) the behavior is the same as previous, using single/multi/in-memory ratio to determine evicting.

      by default, above two changes are both off and the behavior keeps the same as before applying this patch. it's client/user's choice to determine whether or which behavior to use by enabling one of these two enhancements.

      1. HBASE-10263-trunk_v2.patch
        14 kB
        Honghua Feng
      2. HBASE-10263-trunk_v1.patch
        14 kB
        Honghua Feng
      3. HBASE-10263-trunk_v0.patch
        15 kB
        Honghua Feng

        Activity

        Hide
        Honghua Feng added a comment -

        patch attached

        Show
        Honghua Feng added a comment - patch attached
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12620929/HBASE-10263-trunk_v0.patch
        against trunk revision .
        ATTACHMENT ID: 12620929

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.master.TestSplitLogManager

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12620929/HBASE-10263-trunk_v0.patch against trunk revision . ATTACHMENT ID: 12620929 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.master.TestSplitLogManager Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8310//console This message is automatically generated.
        Hide
        Honghua Feng added a comment -

        I re-run unit tests several times locally and all passed.

        Show
        Honghua Feng added a comment - I re-run unit tests several times locally and all passed.
        Hide
        stack added a comment -

        Test failure is likely unrelated. Let me repost the patch to get another hadoopqa run.

        I skimmed the patch. LGTM. Nice test Honghua Feng. I would suggest that this behavior be ON by default in a major release of hbase (0.98 if @apurtell is amenable or 1.0.0 if not); to me, the way this patch is more the 'expected' behavior.

        Show
        stack added a comment - Test failure is likely unrelated. Let me repost the patch to get another hadoopqa run. I skimmed the patch. LGTM. Nice test Honghua Feng . I would suggest that this behavior be ON by default in a major release of hbase (0.98 if @apurtell is amenable or 1.0.0 if not); to me, the way this patch is more the 'expected' behavior.
        Hide
        Ted Yu added a comment -

        In CacheConfig.java :

        +    boolean inMemoryForceMode = conf.getBoolean("hbase.rs.inmemoryforcemode",
        +        false);
        

        Is the above variable used ?

        +   * configuration, inMemoryForceMode is a cluster-wide configuration
        

        Is there plan to make inMemoryForceMode column-family config ?

        Show
        Ted Yu added a comment - In CacheConfig.java : + boolean inMemoryForceMode = conf.getBoolean( "hbase.rs.inmemoryforcemode" , + false ); Is the above variable used ? + * configuration, inMemoryForceMode is a cluster-wide configuration Is there plan to make inMemoryForceMode column-family config ?
        Hide
        Honghua Feng added a comment -

        Ted Yu:

        Is the above variable used(inMemoryForceMode) ?

        ==> No, they(together with single/multi/memory factors) are not used. There is a historical reason for these variables here: this flag(and other 3 factors) will be read from conf passed as parameter in LruBlockCache constructor, in 0.94.3(our internal branch) there is a INFO log for max-size before constructing the LruBlockCache, and I added these 'forceMode/single/multi/memory' info in that INFO log as well, they are used just for info purpose, but this INFO log in CacheConfig.java doesn't exist in trunk code(it's removed), and I forgot to remove these four just-for-info variables accordingly. It won't affect correctness. Thanks for point this out

        Show
        Honghua Feng added a comment - Ted Yu : Is the above variable used(inMemoryForceMode) ? ==> No, they(together with single/multi/memory factors) are not used. There is a historical reason for these variables here: this flag(and other 3 factors) will be read from conf passed as parameter in LruBlockCache constructor, in 0.94.3(our internal branch) there is a INFO log for max-size before constructing the LruBlockCache, and I added these 'forceMode/single/multi/memory' info in that INFO log as well, they are used just for info purpose, but this INFO log in CacheConfig.java doesn't exist in trunk code(it's removed), and I forgot to remove these four just-for-info variables accordingly. It won't affect correctness . Thanks for point this out
        Hide
        Honghua Feng added a comment -

        They are not used in CacheConfig.java, they are read from conf in constructor and surely used in LruBlockCache.java

        Show
        Honghua Feng added a comment - They are not used in CacheConfig.java, they are read from conf in constructor and surely used in LruBlockCache.java
        Hide
        Honghua Feng added a comment -

        new patch removing unused variables in CacheConfig.java per Ted Yu's review, thanks Ted Yu

        Show
        Honghua Feng added a comment - new patch removing unused variables in CacheConfig.java per Ted Yu 's review, thanks Ted Yu
        Hide
        Honghua Feng added a comment -

        stack

        I would suggest that this behavior be ON by default in a major release of hbase (0.98 if @apurtell is amenable or 1.0.0 if not); to me, the way this patch is more the 'expected' behavior.

        ==> the single/multi/memory ratio by default is the same as before(without any tweak): 25%:50%:25%, but user can change them by setting the new configurations, the 'inMemoryForceMode'(preemptive mode for in-memory blocks) is by default OFF, you want to turn 'inMemoryForceMode' ON? hmmm. what about we firstly make it conservative by keeping it OFF by default, and turn it on if we eventually found most of our users tweak it on for their real use
        At least we now provide users a new option to control how 'in-memory' cached blocks mean and behave, and when it's off we enable users to configure the single/multi/memory ratios.
        Opinion?

        Show
        Honghua Feng added a comment - stack I would suggest that this behavior be ON by default in a major release of hbase (0.98 if @apurtell is amenable or 1.0.0 if not); to me, the way this patch is more the 'expected' behavior. ==> the single/multi/memory ratio by default is the same as before(without any tweak): 25%:50%:25%, but user can change them by setting the new configurations, the 'inMemoryForceMode'(preemptive mode for in-memory blocks) is by default OFF, you want to turn 'inMemoryForceMode' ON? hmmm. what about we firstly make it conservative by keeping it OFF by default, and turn it on if we eventually found most of our users tweak it on for their real use At least we now provide users a new option to control how 'in-memory' cached blocks mean and behave, and when it's off we enable users to configure the single/multi/memory ratios. Opinion?
        Hide
        Honghua Feng added a comment -

        Ted Yu

        Is there plan to make inMemoryForceMode column-family config ?

        ==> hmmm...sounds reasonable and feasible, but not sure providing such finer-grained control for this flag is desirable for users. Let me create a new jira for it, and will implementation it if seeing request or someone wants it, thanks for suggestion

        Show
        Honghua Feng added a comment - Ted Yu Is there plan to make inMemoryForceMode column-family config ? ==> hmmm...sounds reasonable and feasible, but not sure providing such finer-grained control for this flag is desirable for users. Let me create a new jira for it, and will implementation it if seeing request or someone wants it, thanks for suggestion
        Hide
        Honghua Feng added a comment -

        Ted Yu, HBASE-10280 is created per your suggestion, please check its description to see if the per-column-family behavior matches your expectation, thanks agaion

        Show
        Honghua Feng added a comment - Ted Yu , HBASE-10280 is created per your suggestion, please check its description to see if the per-column-family behavior matches your expectation, thanks agaion
        Hide
        Ted Yu added a comment -

        Andrew Purtell:
        Do you want this in 0.98 ?

        Show
        Ted Yu added a comment - Andrew Purtell : Do you want this in 0.98 ?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12621447/HBASE-10263-trunk_v1.patch
        against trunk revision .
        ATTACHMENT ID: 12621447

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        -1 release audit. The applied patch generated 4 release audit warnings (more than the trunk's current 0 warnings).

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12621447/HBASE-10263-trunk_v1.patch against trunk revision . ATTACHMENT ID: 12621447 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. -1 release audit . The applied patch generated 4 release audit warnings (more than the trunk's current 0 warnings). +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8338//console This message is automatically generated.
        Hide
        Nick Dimiduk added a comment -

        Evictions happen on a background thread. Filling the cache and then immediately checking the eviction count results in a race between the current thread and the eviction thread; thus this is very likely a flakey test on our over-extended build machines.

        +    // 5th single block
        +    cache.cacheBlock(singleBlocks[4].cacheKey, singleBlocks[4]);
        +    expectedCacheSize += singleBlocks[4].cacheBlockHeapSize();
        +    // Do not expect any evictions yet
        +    assertEquals(0, cache.getEvictionCount());
        +    // Verify cache size
        +    assertEquals(expectedCacheSize, cache.heapSize());
        

        In the above block, the call to cacheBlock() will only notify the eviction thread, not force eviction. A yield or short sleep should be inserted before the call to getEvictionCount() in order to help reduce the chance of exercising the race condition. Repeat for all the following stanzas.

        Show
        Nick Dimiduk added a comment - Evictions happen on a background thread. Filling the cache and then immediately checking the eviction count results in a race between the current thread and the eviction thread; thus this is very likely a flakey test on our over-extended build machines. + // 5th single block + cache.cacheBlock(singleBlocks[4].cacheKey, singleBlocks[4]); + expectedCacheSize += singleBlocks[4].cacheBlockHeapSize(); + // Do not expect any evictions yet + assertEquals(0, cache.getEvictionCount()); + // Verify cache size + assertEquals(expectedCacheSize, cache.heapSize()); In the above block, the call to cacheBlock() will only notify the eviction thread, not force eviction. A yield or short sleep should be inserted before the call to getEvictionCount() in order to help reduce the chance of exercising the race condition. Repeat for all the following stanzas.
        Hide
        Nick Dimiduk added a comment -

        One other stray comment. Your config names should all be modified to reflect that these options are specific to the lru cache. So hbase.rs.inmemoryforcemode, hbase.blockcache.single.percentage, hbase.blockcache.multi.percentage, hbase.blockcache.memory.percentage should all share the common prefix of hbase.lru.blockcache. This follows the precedent established by the existing factor configs.

        Show
        Nick Dimiduk added a comment - One other stray comment. Your config names should all be modified to reflect that these options are specific to the lru cache. So hbase.rs.inmemoryforcemode, hbase.blockcache.single.percentage, hbase.blockcache.multi.percentage, hbase.blockcache.memory.percentage should all share the common prefix of hbase.lru.blockcache. This follows the precedent established by the existing factor configs.
        Hide
        Honghua Feng added a comment -

        Thanks Nick Dimiduk for the careful review

        Evictions happen on a background thread. Filling the cache and then immediately checking the eviction count results in a race between the current thread and the eviction thread; thus this is very likely a flakey test on our over-extended build machines. In the above block, the call to cacheBlock() will only notify the eviction thread, not force eviction.

        What you said is correct for the real usage scenario of LruBlockCache where the evictionThread flag is implicitly true when constructing the LruBlockCache object, that way a background eviction thread is created to do the eviction job, but it's not the case for this newly added unit test: to be able to verify the evict effect of the new configuration/preemptive-mode as quickly as possible without worrying how long to sleep or introducing other kind of synchronization overhead, I disabled the background eviction thread when constructing the LruBlockCache object for this unit test case, this way the eviction will be triggered immediately and synchronously within the cache.cacheBlock call when the cache size exceeds the acceptable cache size.

        LruBlockCache cache = new LruBlockCache(maxSize, blockSize, false...)
        Show
        Honghua Feng added a comment - Thanks Nick Dimiduk for the careful review Evictions happen on a background thread. Filling the cache and then immediately checking the eviction count results in a race between the current thread and the eviction thread; thus this is very likely a flakey test on our over-extended build machines. In the above block, the call to cacheBlock() will only notify the eviction thread, not force eviction. What you said is correct for the real usage scenario of LruBlockCache where the evictionThread flag is implicitly true when constructing the LruBlockCache object, that way a background eviction thread is created to do the eviction job, but it's not the case for this newly added unit test: to be able to verify the evict effect of the new configuration/preemptive-mode as quickly as possible without worrying how long to sleep or introducing other kind of synchronization overhead, I disabled the background eviction thread when constructing the LruBlockCache object for this unit test case, this way the eviction will be triggered immediately and synchronously within the cache.cacheBlock call when the cache size exceeds the acceptable cache size. LruBlockCache cache = new LruBlockCache(maxSize, blockSize, false ...)
        Hide
        Honghua Feng added a comment -

        new patch attached which adds hbase.lru.blockcache prefix to newly introduced config names per Nick Dimiduk's suggestion, thanks Nick Dimiduk

        Show
        Honghua Feng added a comment - new patch attached which adds hbase.lru.blockcache prefix to newly introduced config names per Nick Dimiduk 's suggestion, thanks Nick Dimiduk
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12621740/HBASE-10263-trunk_v2.patch
        against trunk revision .
        ATTACHMENT ID: 12621740

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        -1 release audit. The applied patch generated 4 release audit warnings (more than the trunk's current 0 warnings).

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.regionserver.TestSplitLogWorker

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12621740/HBASE-10263-trunk_v2.patch against trunk revision . ATTACHMENT ID: 12621740 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. -1 release audit . The applied patch generated 4 release audit warnings (more than the trunk's current 0 warnings). +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestSplitLogWorker Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8353//console This message is automatically generated.
        Hide
        Nick Dimiduk added a comment -

        Thanks Honghua Feng for the careful correction of my careless review I do indeed see the evictionThread boolean and your appropriate use of it in the test method.

        Let's get this out in front of people to experiment with, +1 from me!

        Any chance you can post some more detailed performance numbers around this new preemptive mode? Any commentary from your profiling session as to why the existing logic caused such unexpected poor performance? If it's broken always, for everyone, we should follow Good stack's advice and tear out the broken logic, enabling forceInMemory by default.

        Show
        Nick Dimiduk added a comment - Thanks Honghua Feng for the careful correction of my careless review I do indeed see the evictionThread boolean and your appropriate use of it in the test method. Let's get this out in front of people to experiment with, +1 from me! Any chance you can post some more detailed performance numbers around this new preemptive mode? Any commentary from your profiling session as to why the existing logic caused such unexpected poor performance? If it's broken always, for everyone, we should follow Good stack 's advice and tear out the broken logic, enabling forceInMemory by default.
        Hide
        Honghua Feng added a comment -

        Any chance you can post some more detailed performance numbers around this new preemptive mode? Any commentary from your profiling session as to why the existing logic caused such unexpected poor performance? If it's broken always, for everyone, we should follow Good stack's advice and tear out the broken logic, enabling forceInMemory by default.

        Sure, I ever did a series of performance comparison tests for this patch several months ago(it's based on our internal 0.94.3 branch), let me copy the test result from our wiki page to here, wait...

        Show
        Honghua Feng added a comment - Any chance you can post some more detailed performance numbers around this new preemptive mode? Any commentary from your profiling session as to why the existing logic caused such unexpected poor performance? If it's broken always, for everyone, we should follow Good stack's advice and tear out the broken logic, enabling forceInMemory by default. Sure, I ever did a series of performance comparison tests for this patch several months ago(it's based on our internal 0.94.3 branch), let me copy the test result from our wiki page to here, wait...
        Hide
        Honghua Feng added a comment -

        Performance result:

        1. create 2 tables: TA, and TM, all with a single CF, TM's CF is in-memory; disable split to guarantee only 1 region each table; move these 2 tables to a same regionserver
        2. write about 20G data (20M rows * 1K row-size) to each table respectively, major compact after the write is done; the block cache size is 21G;

        Then read: 2 client with 20 threads each issues random read 20G times to these 2 tables...

        1.old version regionserver(fixed 25%:50%:25% single/multi/memory ratio): TA latency=5.5ms; TM latency=11.4ms; (in-memory table performance is about 2 times worse than ordinary table)
        2.new version regionserver(preemptive mode is on): TA latency=19.4ms; TM latency=3.8ms; (now in-memory table performance is much better than ordinary table)

        Since tests against different underlying hardware can yield different test results, but above numbers did prove the improvement effect.
        Additional performance comparison test is encouraged to verify the effect for different hardware configurations

        Show
        Honghua Feng added a comment - Performance result: 1. create 2 tables: TA, and TM, all with a single CF, TM's CF is in-memory; disable split to guarantee only 1 region each table; move these 2 tables to a same regionserver 2. write about 20G data (20M rows * 1K row-size) to each table respectively, major compact after the write is done; the block cache size is 21G; Then read: 2 client with 20 threads each issues random read 20G times to these 2 tables... 1.old version regionserver(fixed 25%:50%:25% single/multi/memory ratio): TA latency=5.5ms; TM latency=11.4ms; (in-memory table performance is about 2 times worse than ordinary table) 2.new version regionserver(preemptive mode is on): TA latency=19.4ms; TM latency=3.8ms; (now in-memory table performance is much better than ordinary table) Since tests against different underlying hardware can yield different test results, but above numbers did prove the improvement effect. Additional performance comparison test is encouraged to verify the effect for different hardware configurations
        Hide
        Liang Xie added a comment -

        +1 from me. It had been integrated into our internal branch long long time ago

        Show
        Liang Xie added a comment - +1 from me. It had been integrated into our internal branch long long time ago
        Hide
        Honghua Feng added a comment -

        Before this jira, a rough performance comparison estimate can be like this(within a single regionserver): suppose the total size of in-memory data served by this regionserver is M, total size of non-in-memory data is N, the block cache size is C, then C/4 is for in-memory data, 3*C/4 is for non-in-memory data, the cache hit ratio of random read for in-memory data is C/(4*M), cache hit ratio for non-in-memory data is 3*C/(4*N), so the performance of random read to these two kinds of data is equal when C/(4*M) == 3*C/(4*N), so:
        1. when M > N/3, in-memory table random read performance is worse than ordinary table;
        2. when M == N/3, in-memory table random read performance is equal to ordinary table;
        2. when M < N/3, in-memory table random read performance is better than ordinary table;

        Show
        Honghua Feng added a comment - Before this jira, a rough performance comparison estimate can be like this(within a single regionserver): suppose the total size of in-memory data served by this regionserver is M, total size of non-in-memory data is N, the block cache size is C, then C/4 is for in-memory data, 3*C/4 is for non-in-memory data, the cache hit ratio of random read for in-memory data is C/(4*M), cache hit ratio for non-in-memory data is 3*C/(4*N), so the performance of random read to these two kinds of data is equal when C/(4*M) == 3*C/(4*N), so: 1. when M > N/3, in-memory table random read performance is worse than ordinary table; 2. when M == N/3, in-memory table random read performance is equal to ordinary table; 2. when M < N/3, in-memory table random read performance is better than ordinary table;
        Hide
        Liang Xie added a comment -

        there're two +1 already. If no new comment/objection, i'd like to commit trunk_v2 into trunk tomorrow.

        Show
        Liang Xie added a comment - there're two +1 already. If no new comment/objection, i'd like to commit trunk_v2 into trunk tomorrow.
        Hide
        Liang Xie added a comment -

        Integrated into trunk. Thanks all for review, thanks making the patch Honghua Feng
        P.S. the release audit was not related with current jira, just checked new jira, should be HBASE-10302 Elliott Clark

        Show
        Liang Xie added a comment - Integrated into trunk. Thanks all for review, thanks making the patch Honghua Feng P.S. the release audit was not related with current jira, just checked new jira, should be HBASE-10302 Elliott Clark
        Hide
        Ted Yu added a comment -

        Honghua:
        Minding filling in release notes ?

        Show
        Ted Yu added a comment - Honghua: Minding filling in release notes ?
        Hide
        Honghua Feng added a comment -

        Ted Yu: you mean to fix the release audit warnings? sure, but the link is not accessible(404) so can't know what the warnings are about

        Show
        Honghua Feng added a comment - Ted Yu : you mean to fix the release audit warnings? sure, but the link is not accessible(404) so can't know what the warnings are about
        Hide
        Ted Yu added a comment -

        When you click the Edit button, you would see input box for Release Notes.
        There are several new config params for this feature, such as hbase.lru.blockcache.single.percentage

        Please document them.

        Thanks

        Show
        Ted Yu added a comment - When you click the Edit button, you would see input box for Release Notes. There are several new config params for this feature, such as hbase.lru.blockcache.single.percentage Please document them. Thanks
        Hide
        Honghua Feng added a comment -

        Ted Yu : done, thanks very much for the kind reminder and instrument

        Show
        Honghua Feng added a comment - Ted Yu : done, thanks very much for the kind reminder and instrument
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4799 (See https://builds.apache.org/job/HBase-TRUNK/4799/)
        HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1556703)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4799 (See https://builds.apache.org/job/HBase-TRUNK/4799/ ) HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1556703) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Hide
        Honghua Feng added a comment -

        thanks Liang Xie

        Show
        Honghua Feng added a comment - thanks Liang Xie
        Hide
        Nick Dimiduk added a comment -

        I'd like this in 0.98. Will commit there as well unless someone strongly objects.

        (cc Andrew Purtell)

        Show
        Nick Dimiduk added a comment - I'd like this in 0.98. Will commit there as well unless someone strongly objects. (cc Andrew Purtell )
        Hide
        stack added a comment -

        hmmm. what about we firstly make it conservative by keeping it OFF by default, and turn it on if we eventually found most of our users tweak it on for their real use

        OK boss.

        Show
        stack added a comment - hmmm. what about we firstly make it conservative by keeping it OFF by default, and turn it on if we eventually found most of our users tweak it on for their real use OK boss.
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-1.1 #47 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/47/)
        HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1556703)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK-on-Hadoop-1.1 #47 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/47/ ) HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1556703) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Hide
        Nick Dimiduk added a comment -

        Actually, Liang Xie do you mind committing also to 0.98? I don't want to steal your thunder on commit

        Show
        Nick Dimiduk added a comment - Actually, Liang Xie do you mind committing also to 0.98? I don't want to steal your thunder on commit
        Hide
        Liang Xie added a comment -

        np sorry for missing your request about merging into 0.98.

        Show
        Liang Xie added a comment - np sorry for missing your request about merging into 0.98.
        Hide
        Liang Xie added a comment -

        reopen to commit to 0.98 as well

        Show
        Liang Xie added a comment - reopen to commit to 0.98 as well
        Hide
        Liang Xie added a comment -

        Nick Dimiduk, just committed into 0.98 branch, thanks for reminder !

        Show
        Liang Xie added a comment - Nick Dimiduk , just committed into 0.98 branch, thanks for reminder !
        Hide
        Andrew Purtell added a comment -

        I don't think it was an unreasonable request to be pinged first as RM for 0.98 Nick Dimiduk, Liang Xie. Next one I start reverting, thanks.

        Show
        Andrew Purtell added a comment - I don't think it was an unreasonable request to be pinged first as RM for 0.98 Nick Dimiduk , Liang Xie . Next one I start reverting, thanks.
        Hide
        Vladimir Rodionov added a comment -

        I have always thought that artificial LruBlockCache divide on regular and in-memory zones was not a good idea. The good cache implementation must sort all these things out itself ... naturally ..

        My 2c.

        Show
        Vladimir Rodionov added a comment - I have always thought that artificial LruBlockCache divide on regular and in-memory zones was not a good idea. The good cache implementation must sort all these things out itself ... naturally .. My 2c.
        Hide
        Liang Xie added a comment -

        got it, andy!

        Show
        Liang Xie added a comment - got it, andy!
        Hide
        Nick Dimiduk added a comment -

        I didn't mean to cause consternation or disrupt your release process, Andrew Purtell. You were pinged by Stack, Ted, and myself on this thread over the last week, so I though you had ample time for you to speak your mind. You're the boss of 0.98 – say the word and I'll revert, no trouble. My apologies.

        Show
        Nick Dimiduk added a comment - I didn't mean to cause consternation or disrupt your release process, Andrew Purtell . You were pinged by Stack, Ted, and myself on this thread over the last week, so I though you had ample time for you to speak your mind. You're the boss of 0.98 – say the word and I'll revert, no trouble. My apologies.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #65 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/65/)
        HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1557298)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #65 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/65/ ) HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1557298) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98 #70 (See https://builds.apache.org/job/HBase-0.98/70/)
        HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1557298)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98 #70 (See https://builds.apache.org/job/HBase-0.98/70/ ) HBASE-10263 make LruBlockCache single/multi/in-memory ratio user-configurable and provide preemptive mode for in-memory type block (Feng Honghua) (liangxie: rev 1557298) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestLruBlockCache.java
        Hide
        Andrew Purtell added a comment -

        I am sorry also that I missed this.

        Show
        Andrew Purtell added a comment - I am sorry also that I missed this.
        Hide
        Andrew Purtell added a comment -

        It's fine to keep in 0.98, btw.

        Show
        Andrew Purtell added a comment - It's fine to keep in 0.98, btw.
        Hide
        Nick Dimiduk added a comment -

        No harm, no foul. Thanks Andrew Purtell!

        Show
        Nick Dimiduk added a comment - No harm, no foul. Thanks Andrew Purtell !
        Hide
        Honghua Feng added a comment -

        Vladimir Rodionov :

        I have always thought that artificial LruBlockCache divide on regular and in-memory zones was not a good idea.

        to some extent, I agree

        The good cache implementation must sort all these things out itself

        not quite...for some applications they need better latency for some table regardless of what the table's access pattern is compared to other tables. treating all the tables the same way and just letting the 'good' cache take care of all of these is not desirable for such applications. for example think about the META table(which is the internal in-memory table)

        Show
        Honghua Feng added a comment - Vladimir Rodionov : I have always thought that artificial LruBlockCache divide on regular and in-memory zones was not a good idea. to some extent, I agree The good cache implementation must sort all these things out itself not quite...for some applications they need better latency for some table regardless of what the table's access pattern is compared to other tables. treating all the tables the same way and just letting the 'good' cache take care of all of these is not desirable for such applications. for example think about the META table(which is the internal in-memory table)
        Hide
        Enis Soztutar added a comment -

        Closing this issue after 0.99.0 release.

        Show
        Enis Soztutar added a comment - Closing this issue after 0.99.0 release.

          People

          • Assignee:
            Honghua Feng
            Reporter:
            Honghua Feng
          • Votes:
            0 Vote for this issue
            Watchers:
            15 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development