Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-9769

Improve performance of a Scanner with explicit column list when rows are small/medium size

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Duplicate
    • Affects Version/s: 0.98.0, 0.94.12, 0.96.0
    • Fix Version/s: None
    • Component/s: Scanners
    • Labels:
      None
    • Release Note:
      Hide
      The feature allows to improve performance of scan operations when table rows are small/medium size and scan operation contains explicit set of column:qualifiers to filter on. By default, the feature is disabled. To enable it set Scan.HINT_NARROW_ROWS attribute of Scan object instance:
      Scan scan = ...
      scan.setAttribute(Scan.HINT_NARROW_ROWS, Bytes.toBytes(Boolean.TRUE));

      The performance improvement depends on a average row size. The smaller rows are - the larger performance improvement is.
      Show
      The feature allows to improve performance of scan operations when table rows are small/medium size and scan operation contains explicit set of column:qualifiers to filter on. By default, the feature is disabled. To enable it set Scan.HINT_NARROW_ROWS attribute of Scan object instance: Scan scan = ... scan.setAttribute(Scan.HINT_NARROW_ROWS, Bytes.toBytes(Boolean.TRUE)); The performance improvement depends on a average row size. The smaller rows are - the larger performance improvement is.
    1. 9769-0.94-sample.txt
      1 kB
      Lars Hofhansl
    2. 9769-0.94-sample1.txt
      1 kB
      Lars Hofhansl
    3. 9769-0.94-sample2.txt
      0.9 kB
      Lars Hofhansl
    4. 9769-94.txt
      9 kB
      Vladimir Rodionov
    5. 9769-94-v2.txt
      9 kB
      Vladimir Rodionov
    6. 9769-trunk-v1.txt
      8 kB
      Vladimir Rodionov
    7. 9769-trunk-v2.txt
      12 kB
      Vladimir Rodionov
    8. 9769-trunk-v3.txt
      13 kB
      Vladimir Rodionov
    9. 9769-trunk-v4.txt
      12 kB
      Vladimir Rodionov

      Issue Links

        Activity

        Hide
        lhofhansl Lars Hofhansl added a comment -

        This should be fixed now in a general way with HBASE-13109.
        Feel free to reopen if that is not the case.

        Show
        lhofhansl Lars Hofhansl added a comment - This should be fixed now in a general way with HBASE-13109 . Feel free to reopen if that is not the case.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Yes, its 1 HFile and all data was cached in block cache.

        Show
        vrodionov Vladimir Rodionov added a comment - Yes, its 1 HFile and all data was cached in block cache.
        Hide
        stepinto Chao Shi added a comment -

        Hi folks,

        I'm running into a similar problem (HBASE-9811) and I got some interesting testing figures (in that ticket). Briefly speaking, 1) I get similar improvement when replace SEEK_NEXT_COL with SKIP and 2) the performance drops greatly as we get more HFiles. So did your test case use only 1 HFile, Vladimir Rodionov?

        Show
        stepinto Chao Shi added a comment - Hi folks, I'm running into a similar problem ( HBASE-9811 ) and I got some interesting testing figures (in that ticket). Briefly speaking, 1) I get similar improvement when replace SEEK_NEXT_COL with SKIP and 2) the performance drops greatly as we get more HFiles. So did your test case use only 1 HFile, Vladimir Rodionov ?
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Yes, I can confirm that on scan operations only reseeks are used and HBASE-5987 works in 0.94 upwards. May be there is not much sense in this optimization for seekTo (as since it is for initial scan setup and we need anyway to go through block index)?

        Show
        vrodionov Vladimir Rodionov added a comment - Yes, I can confirm that on scan operations only reseeks are used and HBASE-5987 works in 0.94 upwards. May be there is not much sense in this optimization for seekTo (as since it is for initial scan setup and we need anyway to go through block index)?
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Let's do a new issue. HBASE-5987 is for reseek only as we know we only scan forward in that case.
        So it looks like HBASE-5987 is working as expected for reseeks.

        Show
        lhofhansl Lars Hofhansl added a comment - Let's do a new issue. HBASE-5987 is for reseek only as we know we only scan forward in that case. So it looks like HBASE-5987 is working as expected for reseeks.
        Hide
        stack stack added a comment -

        Or rather than reopen we should do a new issue.

        Show
        stack stack added a comment - Or rather than reopen we should do a new issue.
        Hide
        stack stack added a comment -

        HBASE-5987 was forward-ported as HBASE-6032 I reopened it.

        Show
        stack stack added a comment - HBASE-5987 was forward-ported as HBASE-6032 I reopened it.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        I think that HBASE-5987 does not work in 0.94 and trunk. This is why we are doing all this scanners hints and optimizations.

        Show
        vrodionov Vladimir Rodionov added a comment - I think that HBASE-5987 does not work in 0.94 and trunk. This is why we are doing all this scanners hints and optimizations.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Looks like HBase-5987 and related JIRAs need to be reopened. Can somebody go through HFileScanner's hierarchy and confirm my findings?

        Show
        vrodionov Vladimir Rodionov added a comment - Looks like HBase-5987 and related JIRAs need to be reopened. Can somebody go through HFileScanner's hierarchy and confirm my findings?
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Interesting. We check if sought key is inside current block only in AbstractScannerV2.reseekTo. There are other public methods exposed by HFileScanner and implemented in AbstractScannerV2 : seekTo and seekBefore, which do not check current block and always goes to index.

        Show
        vrodionov Vladimir Rodionov added a comment - Interesting. We check if sought key is inside current block only in AbstractScannerV2.reseekTo . There are other public methods exposed by HFileScanner and implemented in AbstractScannerV2 : seekTo and seekBefore, which do not check current block and always goes to index.
        Hide
        stack stack added a comment -

        Do we see the benefit chunhui shen talks of?

        Show
        stack stack added a comment - Do we see the benefit chunhui shen talks of?
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Never mind, I was wrong. The code is correct.

        Show
        vrodionov Vladimir Rodionov added a comment - Never mind, I was wrong. The code is correct.
        Hide
        stack stack added a comment -

        Perhaps this was incorrectly forward-ported?

        Show
        stack stack added a comment - Perhaps this was incorrectly forward-ported?
        Hide
        vrodionov Vladimir Rodionov added a comment -

        I just checked trunk AbstractScannerV2. The code of HBase-5987 is there:

            @Override
            public int reseekTo(byte[] key, int offset, int length) throws IOException {
              int compared;
              if (isSeeked()) {
                ByteBuffer bb = getKey();
                compared = reader.getComparator().compare(key, offset,
                    length, bb.array(), bb.arrayOffset(), bb.limit());
                if (compared < 1) {
                  // If the required key is less than or equal to current key, then
                  // don't do anything.
                  return compared;
                } else {
                  if (this.nextIndexedKey != null &&
                      (this.nextIndexedKey == HConstants.NO_NEXT_INDEXED_KEY ||
                       reader.getComparator().compare(key, offset, length,
                           nextIndexedKey, 0, nextIndexedKey.length) < 0)) {
                    // The reader shall continue to scan the current data block instead of querying the
                    // block index as long as it knows the target key is strictly smaller than
                    // the next indexed key or the current data block is the last data block.
                    return loadBlockAndSeekToKey(this.block, this.nextIndexedKey,
                        false, key, offset, length, false);
                  }
                }
              }
              // Don't rewind on a reseek operation, because reseek implies that we are
              // always going forward in the file.
              return seekTo(key, offset, length, false);
            }
        

        but it seems that nextIndexedKey is never initialized properly. I did not manage to find the place where the next block first key is assigned to this variable.

        Show
        vrodionov Vladimir Rodionov added a comment - I just checked trunk AbstractScannerV2. The code of HBase-5987 is there: @Override public int reseekTo( byte [] key, int offset, int length) throws IOException { int compared; if (isSeeked()) { ByteBuffer bb = getKey(); compared = reader.getComparator().compare(key, offset, length, bb.array(), bb.arrayOffset(), bb.limit()); if (compared < 1) { // If the required key is less than or equal to current key, then // don't do anything. return compared; } else { if ( this .nextIndexedKey != null && ( this .nextIndexedKey == HConstants.NO_NEXT_INDEXED_KEY || reader.getComparator().compare(key, offset, length, nextIndexedKey, 0, nextIndexedKey.length) < 0)) { // The reader shall continue to scan the current data block instead of querying the // block index as long as it knows the target key is strictly smaller than // the next indexed key or the current data block is the last data block. return loadBlockAndSeekToKey( this .block, this .nextIndexedKey, false , key, offset, length, false ); } } } // Don't rewind on a reseek operation, because reseek implies that we are // always going forward in the file. return seekTo(key, offset, length, false ); } but it seems that nextIndexedKey is never initialized properly. I did not manage to find the place where the next block first key is assigned to this variable.
        Hide
        stack stack added a comment -

        Isn't HBASE-5987 committed on 0.94? If so, I wonder why we do not see the benefit chunhui shen talks of.

        Show
        stack stack added a comment - Isn't HBASE-5987 committed on 0.94? If so, I wonder why we do not see the benefit chunhui shen talks of.
        Hide
        zjushch chunhui shen added a comment -

        HBASE-5987 would greatly improve the performance for this case , I think.

        'reseek' is optimizated in HBASE-5987.

        +            // The reader shall continue to scan the current data block instead of querying the
        +            // block index as long as it knows the target key is strictly smaller than
        +            // the next indexed key or the current data block is the last data block.
        

        In addition, Performace difference is not so much between expliciting column list and scaning wildcard columns in my test environment. I think it's the effect of HBASE-5987 since we applied it

        Show
        zjushch chunhui shen added a comment - HBASE-5987 would greatly improve the performance for this case , I think. 'reseek' is optimizated in HBASE-5987 . + // The reader shall continue to scan the current data block instead of querying the + // block index as long as it knows the target key is strictly smaller than + // the next indexed key or the current data block is the last data block. In addition, Performace difference is not so much between expliciting column list and scaning wildcard columns in my test environment. I think it's the effect of HBASE-5987 since we applied it
        Hide
        stack stack added a comment -

        Patch looks good caveat above questions. It was better having the filter in regionserver package as you originally had it – then its access could have been shutdown confined to where it is used. I like what Lars Hofhansl says about "...if that the column tracker code is not efficient we should fix that rather than circumventing it completely with a filter." Doc of this public static could be better explaining when a user would set the attribute:

        • + /** Scan Hints */
          + static public final String HINT_NARROW_ROWS = "hint_narrow_rows";

        Good stuff Vladimir.

        Show
        stack stack added a comment - Patch looks good caveat above questions. It was better having the filter in regionserver package as you originally had it – then its access could have been shutdown confined to where it is used. I like what Lars Hofhansl says about "...if that the column tracker code is not efficient we should fix that rather than circumventing it completely with a filter." Doc of this public static could be better explaining when a user would set the attribute: + /** Scan Hints */ + static public final String HINT_NARROW_ROWS = " hint_narrow_rows "; Good stuff Vladimir.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Only with a coprocessor would it be possible to exercise checkVersion and avoid the network IO.
        This is exactly what I am interested in - optimizing scan operation in coprocessors (think - Phoenix). I will try your patch when I have a time this week.

        Show
        vrodionov Vladimir Rodionov added a comment - Only with a coprocessor would it be possible to exercise checkVersion and avoid the network IO. This is exactly what I am interested in - optimizing scan operation in coprocessors (think - Phoenix). I will try your patch when I have a time this week.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        We should test end-to-end, not a microbenchmark of StoreScanner.

        Note that you cannot exercise the seeking code in checkVersion without returning data to the client, in which case network IO will dominate. If you filter KVs out with a filter before that checkVersion is never called, if the filter returns INCLUDE it'll call checkVersion and incur a seek. Only with a coprocessor would it be possible to exercise checkVersion and avoid the network IO.

        Also note that in your filter case you'd still get the SEEK_NEXT_ROW/SEEK_NEXT_COL in ScanWildcardColumnTracker.checkVersion for each column that you included.

        When you get a chance, could you check out the last patch on HBASE-9778? Maybe you could run it through your micro StoreScanner test, I'd be curious how it compares.

        Generally, if that the column tracker code is not efficient we should fix that rather than circumventing it completely with a filter.

        Show
        lhofhansl Lars Hofhansl added a comment - We should test end-to-end, not a microbenchmark of StoreScanner. Note that you cannot exercise the seeking code in checkVersion without returning data to the client, in which case network IO will dominate. If you filter KVs out with a filter before that checkVersion is never called, if the filter returns INCLUDE it'll call checkVersion and incur a seek. Only with a coprocessor would it be possible to exercise checkVersion and avoid the network IO. Also note that in your filter case you'd still get the SEEK_NEXT_ROW/SEEK_NEXT_COL in ScanWildcardColumnTracker.checkVersion for each column that you included. When you get a chance, could you check out the last patch on HBASE-9778 ? Maybe you could run it through your micro StoreScanner test, I'd be curious how it compares. Generally, if that the column tracker code is not efficient we should fix that rather than circumventing it completely with a filter.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Performance-wise, I think this filter is going to be faster than ExplicitColumnTracker with a hint. To make it comparable to the ExplicitScanReplacementFilter, you will have to optimize the ExplicitColumnTracker's code.

        Show
        vrodionov Vladimir Rodionov added a comment - Performance-wise, I think this filter is going to be faster than ExplicitColumnTracker with a hint. To make it comparable to the ExplicitScanReplacementFilter, you will have to optimize the ExplicitColumnTracker's code.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Lastly (and sorry for being difficult), why is this faster than passing the small row hint to ExplicitColumnTracker and replace SEEK_NEXT_COL with SKIP? (this would be HBASE-9778 but with your explicit hint)

        It seems the ExplicitColumnTracker then does close to the same work as ScanWildcardColumnTracker.

        Show
        lhofhansl Lars Hofhansl added a comment - Lastly (and sorry for being difficult), why is this faster than passing the small row hint to ExplicitColumnTracker and replace SEEK_NEXT_COL with SKIP? (this would be HBASE-9778 but with your explicit hint) It seems the ExplicitColumnTracker then does close to the same work as ScanWildcardColumnTracker.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Some comments:

        • shouldUseExplicitColumnFilter is misnamed. It has the side effect of adding the filter.
        • Curious how much slower just using HashMap was instead of having your own bucket array.
        • I think this would be cleaner if the Filter would be a proper filter that can serialized (i.e. protoful in trunk and readFields/write in 0.94). (FYI. I am debating the same in HBASE-9272. The parallel scanner could just be a sample scanner to use, or it could automatically triggered, but it is still 100% client side in either case)
        Show
        lhofhansl Lars Hofhansl added a comment - Some comments: shouldUseExplicitColumnFilter is misnamed. It has the side effect of adding the filter. Curious how much slower just using HashMap was instead of having your own bucket array. I think this would be cleaner if the Filter would be a proper filter that can serialized (i.e. protoful in trunk and readFields/write in 0.94). (FYI. I am debating the same in HBASE-9272 . The parallel scanner could just be a sample scanner to use, or it could automatically triggered, but it is still 100% client side in either case)
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Since HStore is modified, this feature is not totally client side.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Since HStore is modified, this feature is not totally client side.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        I prefer keeping the existing version, of course. The reason is new Scan hinting system. This is the first performance - oriented HINT for Scan operation. There are some others coming.

        Show
        vrodionov Vladimir Rodionov added a comment - I prefer keeping the existing version, of course. The reason is new Scan hinting system. This is the first performance - oriented HINT for Scan operation. There are some others coming.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Sure, it will be slower for rows above 1-2K in size. I have not done any testing on max row size, but 5 cols rows of 150 bytes total is much faster with the filter. The filter is not client - side.

        Show
        vrodionov Vladimir Rodionov added a comment - Sure, it will be slower for rows above 1-2K in size. I have not done any testing on max row size, but 5 cols rows of 150 bytes total is much faster with the filter. The filter is not client - side.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        I would prefer if we just included the Filter and document its use.
        Generally this approach will be slower with many columns or many versions of few columns.

        What do you think about that (Vladimir Rodionov, stack, Ted Yu)?

        Show
        lhofhansl Lars Hofhansl added a comment - I would prefer if we just included the Filter and document its use. Generally this approach will be slower with many columns or many versions of few columns. What do you think about that ( Vladimir Rodionov , stack , Ted Yu )?
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        +1 from me.

        Mind adding release note ?

        Show
        yuzhihong@gmail.com Ted Yu added a comment - +1 from me. Mind adding release note ?
        Hide
        vrodionov Vladimir Rodionov added a comment -

        This patch has nothing to do with failed Zk test.

        Show
        vrodionov Vladimir Rodionov added a comment - This patch has nothing to do with failed Zk test.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12609099/9769-trunk-v4.txt
        against trunk revision .

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

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

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

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 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 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.zookeeper.lock.TestZKInterProcessReadWriteLock

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

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12609099/9769-trunk-v4.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 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 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.zookeeper.lock.TestZKInterProcessReadWriteLock Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7587//console This message is automatically generated.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Fixed small type, sorry. New patch v4.

        Show
        vrodionov Vladimir Rodionov added a comment - Fixed small type, sorry. New patch v4.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12609092/9769-trunk-v3.txt
        against trunk revision .

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

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

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

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 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 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 passed unit tests in .

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

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12609092/9769-trunk-v3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 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 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7586//console This message is automatically generated.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        trunk v3

        Show
        vrodionov Vladimir Rodionov added a comment - trunk v3
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        For ExplicitScanReplacementFilter:

        + * Copyright 2013 The Apache Software Foundation
        

        Year is not needed.

        +package org.apache.hadoop.hbase.regionserver;
        

        I thought you were going to move this class to filter package.

        -  private abstract static class SinkWriter {
        +  static class SinkWriter {
        

        Is the above needed for this JIRA ?

        +  private static byte[][] CQ = new byte[][]{
        

        nit: since CQ holds qualifiers, consider naming the variable CQs.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - For ExplicitScanReplacementFilter: + * Copyright 2013 The Apache Software Foundation Year is not needed. + package org.apache.hadoop.hbase.regionserver; I thought you were going to move this class to filter package. - private abstract static class SinkWriter { + static class SinkWriter { Is the above needed for this JIRA ? + private static byte [][] CQ = new byte [][]{ nit: since CQ holds qualifiers, consider naming the variable CQs.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12609076/9769-trunk-v2.txt
        against trunk revision .

        +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 hadoop2.0. The patch compiles against the hadoop 2.0 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 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 passed unit tests in .

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

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12609076/9769-trunk-v2.txt against trunk revision . +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 hadoop2.0 . The patch compiles against the hadoop 2.0 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 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7584//console This message is automatically generated.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        New patch submitted.

        Show
        vrodionov Vladimir Rodionov added a comment - New patch submitted.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Trunk patch v2.

        Show
        vrodionov Vladimir Rodionov added a comment - Trunk patch v2.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Minor comment:

        +    for (int i = 0; i < length; i++) {
        +      h = 31 * h + buffer[off++];
        

        Both i and off are incremented in each iteration. Looks like 'off++' can be replaced with 'offset+i'.

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Minor comment: + for ( int i = 0; i < length; i++) { + h = 31 * h + buffer[off++]; Both i and off are incremented in each iteration. Looks like 'off++' can be replaced with 'offset+i'.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Did some profiling on why reseek() is so much slower than next() even when reseek just has to seek to the next key. The reason is all the compares we're doing... For each reseek:

        • 2 KV compares in KeyValueHeap.generalizedSeek to find the top scanner
        • 2 key compares in HFileReaderV2.ScannerV2.reseekTo (one to check for reseek, one to check against the index key)
        • 2 key compares in HFileReaderV2.ScannerV2.blockSeek to find the right key

        After all that we finally read the KV we found.

        While next() just reads the next KV from the current HFile block.

        Nothing jumps here as to how we could simplify this.

        Show
        lhofhansl Lars Hofhansl added a comment - Did some profiling on why reseek() is so much slower than next() even when reseek just has to seek to the next key. The reason is all the compares we're doing... For each reseek: 2 KV compares in KeyValueHeap.generalizedSeek to find the top scanner 2 key compares in HFileReaderV2.ScannerV2.reseekTo (one to check for reseek, one to check against the index key) 2 key compares in HFileReaderV2.ScannerV2.blockSeek to find the right key After all that we finally read the KV we found. While next() just reads the next KV from the current HFile block. Nothing jumps here as to how we could simplify this.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        1. Its not a client side filter (keeps only column qualifiers - no column families). I decided to put it into regionserver, but I can move to hbase-client
        2. OK, shouldUse is better
        3. you right this check is not needed.

        I will add tests, of course.

        Show
        vrodionov Vladimir Rodionov added a comment - 1. Its not a client side filter (keeps only column qualifiers - no column families). I decided to put it into regionserver, but I can move to hbase-client 2. OK, shouldUse is better 3. you right this check is not needed. I will add tests, of course.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Please add annotation for audience and stability:

        +public class ExplicitScanReplacementFilter extends FilterBase {
        

        Should it be in org.apache.hadoop.hbase.filter package ?

        +package org.apache.hadoop.hbase.regionserver;
        
        +  private boolean doesUseExplicitColumnFilter(Scan scan) {
        

        Name the method shouldUseExplicitColumnFilter() ?

        +      if (cols != null && (cols.size() > 1 || cols.first() != null)) {
        

        Why is cols.size() > 1 check needed ?

        Can you add a test for the new class ?

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Please add annotation for audience and stability: + public class ExplicitScanReplacementFilter extends FilterBase { Should it be in org.apache.hadoop.hbase.filter package ? + package org.apache.hadoop.hbase.regionserver; + private boolean doesUseExplicitColumnFilter(Scan scan) { Name the method shouldUseExplicitColumnFilter() ? + if (cols != null && (cols.size() > 1 || cols.first() != null )) { Why is cols.size() > 1 check needed ? Can you add a test for the new class ?
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12608840/9769-trunk-v1.txt
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

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

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 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 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 passed unit tests in .

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

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12608840/9769-trunk-v1.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 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 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7574//console This message is automatically generated.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Trunk patch v1.

        Show
        vrodionov Vladimir Rodionov added a comment - Trunk patch v1.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        This is what I meant with 'trunk':
        http://svn.apache.org/repos/asf/hbase/trunk

        Show
        yuzhihong@gmail.com Ted Yu added a comment - This is what I meant with 'trunk': http://svn.apache.org/repos/asf/hbase/trunk
        Hide
        vrodionov Vladimir Rodionov added a comment -

        I thought that I created patch for 0.94-trunk (which I created with branch 0.94).

        Show
        vrodionov Vladimir Rodionov added a comment - I thought that I created patch for 0.94-trunk (which I created with branch 0.94).
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        QA only applies patch on HBase trunk.

        Can you attach patch for trunk ?

        Show
        yuzhihong@gmail.com Ted Yu added a comment - QA only applies patch on HBase trunk. Can you attach patch for trunk ?
        Hide
        vrodionov Vladimir Rodionov added a comment -

        What does The patch command could not apply the patch. mean?
        I used

        git diff --no-prefix > patch.txt
        to create the patch on 94-trunk.

        Show
        vrodionov Vladimir Rodionov added a comment - What does The patch command could not apply the patch. mean? I used git diff --no-prefix > patch.txt to create the patch on 94-trunk.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12608773/9769-94-v2.txt
        against trunk revision .

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7570//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12608773/9769-94-v2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7570//console This message is automatically generated.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        First patch for 94-trunk.

        Show
        vrodionov Vladimir Rodionov added a comment - First patch for 94-trunk.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Patch v2. Replaced tab/with spaces, renamed Hint to HINT_NARROW_ROWS, removed check for MAX_VERSIONS (its in in a different JIRA).

        Show
        vrodionov Vladimir Rodionov added a comment - Patch v2. Replaced tab/with spaces, renamed Hint to HINT_NARROW_ROWS, removed check for MAX_VERSIONS (its in in a different JIRA).
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Some additional performance numbers on a patch:

        Table: 1 CF + 5 CQ, value ~ 10-15 bytes. Rows = 500000. All data in a block cache. Tested on *StoreScanner * directly.

        Default:
        Raw = 1.28M rows per sec
        1 CQ in Scan = 0.7M
        2 CQ in Scan = 0.5M
        3 CQ in Scan = 0.4M
        4 CQ in Scan = 0.32M
        5 CQ in Scan = 0.33M

        Patch:

        Raw = 1.28M rows per sec
        1 CQ in Scan = 1.27M
        2 CQ in Scan = 1.2M
        3 CQ in Scan = 1.1M
        4 CQ in Scan = 1.05M
        5 CQ in Scan = 1M

        Show
        vrodionov Vladimir Rodionov added a comment - Some additional performance numbers on a patch: Table: 1 CF + 5 CQ, value ~ 10-15 bytes. Rows = 500000. All data in a block cache. Tested on *StoreScanner * directly. Default: Raw = 1.28M rows per sec 1 CQ in Scan = 0.7M 2 CQ in Scan = 0.5M 3 CQ in Scan = 0.4M 4 CQ in Scan = 0.32M 5 CQ in Scan = 0.33M Patch: Raw = 1.28M rows per sec 1 CQ in Scan = 1.27M 2 CQ in Scan = 1.2M 3 CQ in Scan = 1.1M 4 CQ in Scan = 1.05M 5 CQ in Scan = 1M
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Nit: Use the Eclipse formatter from HBASE-5961. We use 2 spaces instead of a tab for indentation.

        Show
        lhofhansl Lars Hofhansl added a comment - Nit: Use the Eclipse formatter from HBASE-5961 . We use 2 spaces instead of a tab for indentation.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        I created HBASE-9778 for my patch idea.

        Show
        lhofhansl Lars Hofhansl added a comment - I created HBASE-9778 for my patch idea.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        MAX_VERSIONS=1 (or a low number) can only be used to eliminate the NEXT_COL seek (as that is use to seek past versions of the same column). It does not indicate anything about the number of columns in a row, and hence we know nothing about whether SEEK_NEXT_ROW or a series of SKIPs is better.

        We need both, I think.

        (MAX_VERSIONS is a hint in the sense that there temporarily can be more versions in the memstore and/or distributed over various HFiles, only after a major compaction will the number of versions actually be <= MAX_VERSIONS.)

        Show
        lhofhansl Lars Hofhansl added a comment - MAX_VERSIONS=1 (or a low number) can only be used to eliminate the NEXT_COL seek (as that is use to seek past versions of the same column). It does not indicate anything about the number of columns in a row, and hence we know nothing about whether SEEK_NEXT_ROW or a series of SKIPs is better. We need both, I think. (MAX_VERSIONS is a hint in the sense that there temporarily can be more versions in the memstore and/or distributed over various HFiles, only after a major compaction will the number of versions actually be <= MAX_VERSIONS.)
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Lars, our patches are independent. I think they need to be merged into one, or you better create new JIRA for do-not-seek-next-col thing.

        Show
        vrodionov Vladimir Rodionov added a comment - Lars, our patches are independent. I think they need to be merged into one, or you better create new JIRA for do-not-seek-next-col thing.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        To activate this feature (hint):

        Scan scan = ...
            scan.setAttribute(Scan.SCAN_NARROW_ROWS, "true".getBytes());
        

        OK. I think I will replace SCAN_NARROW_ROWS with HINT_NARROW_ROWS.

        Show
        vrodionov Vladimir Rodionov added a comment - To activate this feature (hint): Scan scan = ... scan.setAttribute(Scan.SCAN_NARROW_ROWS, " true " .getBytes()); OK. I think I will replace SCAN_NARROW_ROWS with HINT_NARROW_ROWS.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        It contains check MAX_VERSIONS = 1 suggested by Lars (not sure if it is really a hint?). Lars version gives improvements as well, but it relies on default hint of MAX_VERSIONS and is slower I think. I completely eliminated ExplicitColumnTracker from the code path. The more columns in a scan the more is going to be performance difference, I think (again).

        Show
        vrodionov Vladimir Rodionov added a comment - It contains check MAX_VERSIONS = 1 suggested by Lars (not sure if it is really a hint?). Lars version gives improvements as well, but it relies on default hint of MAX_VERSIONS and is slower I think. I completely eliminated ExplicitColumnTracker from the code path. The more columns in a scan the more is going to be performance difference, I think (again).
        Hide
        vrodionov Vladimir Rodionov added a comment -

        It contains check MAX_VERSIONS = 1 suggested by Lars (not sure if it is really a hint?).

        Show
        vrodionov Vladimir Rodionov added a comment - It contains check MAX_VERSIONS = 1 suggested by Lars (not sure if it is really a hint?).
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Initial version of a patch

        Show
        vrodionov Vladimir Rodionov added a comment - Initial version of a patch
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Lars, HTable can have small number of versions and large number of column qualifiers or large values (say 100K).

        That is true. Seeking to the next column is not a good idea, though, if we know there are not going to be many versions to skip. So the suggested patch here will not be slower than before, and it will improve performance in many cases.

        As the size of a KV approaches the HFile blocksize (64k by default), SKIP and SEEK_NEXT_COL should become equivalent in performance (in both cases we'll need to find the KV in the next block).

        As I said, this does not eliminate the NEXT_ROW seeking.

        I fear the filter approach will lead to issues when there are already filters configured on the scan. You'd have to convert this to a FilterList while keeping all the semantics and performance characteristics.
        I think it might be best to ship your Filter and document its use.

        I'll file a separate issue for my patch.

        Show
        lhofhansl Lars Hofhansl added a comment - Lars, HTable can have small number of versions and large number of column qualifiers or large values (say 100K). That is true. Seeking to the next column is not a good idea, though, if we know there are not going to be many versions to skip. So the suggested patch here will not be slower than before, and it will improve performance in many cases. As the size of a KV approaches the HFile blocksize (64k by default), SKIP and SEEK_NEXT_COL should become equivalent in performance (in both cases we'll need to find the KV in the next block). As I said, this does not eliminate the NEXT_ROW seeking. I fear the filter approach will lead to issues when there are already filters configured on the scan. You'd have to convert this to a FilterList while keeping all the semantics and performance characteristics. I think it might be best to ship your Filter and document its use. I'll file a separate issue for my patch.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Note that Vladimir's small row hint still can be used to eliminate the NEXT_ROW seek. Maybe, again, it is prudent to split this in two issues.

        Show
        lhofhansl Lars Hofhansl added a comment - Note that Vladimir's small row hint still can be used to eliminate the NEXT_ROW seek. Maybe, again, it is prudent to split this in two issues.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Even simpler. Same perf numbers.
        (The previous patch was actually not correct when there are actually multiple versions).

        Show
        lhofhansl Lars Hofhansl added a comment - Even simpler. Same perf numbers. (The previous patch was actually not correct when there are actually multiple versions).
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Interestingly it depends on which column(s) is (are) selected.

        Some numbers: 4m rows, 5 cols each, 1 cf, 10 bytes values, VERSIONS=1. Everything measured in seconds.

        Without patch:

        Wildcard Col 1 Col 2 Col 4 Col 5 Col 2+4
        6.4 8.5 14.3 14.6 11.1 20.3

        With patch sample1:

        Wildcard Col 1 Col 2 Col 4 Col 5 Col 2+4
        6.4 8.4 8.9 9.9 6.4 10.0

        Variation here was +- 0.2s.

        So with this patch scanning is 2x faster than without in some cases, and never slower. No special hint needed, beyond declaring VERSIONS correctly.

        Show
        lhofhansl Lars Hofhansl added a comment - Interestingly it depends on which column(s) is (are) selected. Some numbers: 4m rows, 5 cols each, 1 cf, 10 bytes values, VERSIONS=1. Everything measured in seconds. Without patch: Wildcard Col 1 Col 2 Col 4 Col 5 Col 2+4 6.4 8.5 14.3 14.6 11.1 20.3 With patch sample1: Wildcard Col 1 Col 2 Col 4 Col 5 Col 2+4 6.4 8.4 8.9 9.9 6.4 10.0 Variation here was +- 0.2s. So with this patch scanning is 2x faster than without in some cases, and never slower. No special hint needed, beyond declaring VERSIONS correctly.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        With VERSIONS=1 only. I think I like this better. User declares to only be interested in 1 version, in that case we use SKIP and INCLUDE and not column seeking.

        Show
        lhofhansl Lars Hofhansl added a comment - With VERSIONS=1 only. I think I like this better. User declares to only be interested in 1 version, in that case we use SKIP and INCLUDE and not column seeking.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Lars, HTable can have small number of versions and large number of column qualifiers or large values (say 100K).

        Show
        vrodionov Vladimir Rodionov added a comment - Lars, HTable can have small number of versions and large number of column qualifiers or large values (say 100K).
        Hide
        lhofhansl Lars Hofhansl added a comment - - edited

        Here's a sample of what I mean.
        10 is hardcoded. Alternatively we could only do this when VERSIONS=1, so this way the user would naturally provide a hint about how many version (s)he expects to keep and we use that hint.

        Show
        lhofhansl Lars Hofhansl added a comment - - edited Here's a sample of what I mean. 10 is hardcoded. Alternatively we could only do this when VERSIONS=1, so this way the user would naturally provide a hint about how many version (s)he expects to keep and we use that hint.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Another idea I had was to make use of the column family's VERSIONS setting. If it is "small" use INCLUDE and SKIP in the ExplicitColumnTracker, otherwise use INCLUDE_AND_SEEK_NEXT_COL and SEEK_NEXT_COL. In my tests this yields a nice improvement bringing ExplicitColumnTracker on par with ScanWildcardColumnTracker. For now I defined "small" as 10, but that needs to be tested more.

        Show
        lhofhansl Lars Hofhansl added a comment - Another idea I had was to make use of the column family's VERSIONS setting. If it is "small" use INCLUDE and SKIP in the ExplicitColumnTracker, otherwise use INCLUDE_AND_SEEK_NEXT_COL and SEEK_NEXT_COL. In my tests this yields a nice improvement bringing ExplicitColumnTracker on par with ScanWildcardColumnTracker. For now I defined "small" as 10, but that needs to be tested more.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Its a server-side filter and is not meant to be exposed to HBase client. The reason: it has only list of qualifiers - no columns. It is instantiated in StoreScanner. If Scan has already filter, the new FilterList is created with MUST_PASS_ALL operator. First goes ExplicitColumnsFilter then existing filter.

        Show
        vrodionov Vladimir Rodionov added a comment - Its a server-side filter and is not meant to be exposed to HBase client. The reason: it has only list of qualifiers - no columns. It is instantiated in StoreScanner. If Scan has already filter, the new FilterList is created with MUST_PASS_ALL operator. First goes ExplicitColumnsFilter then existing filter.
        Hide
        zjushch chunhui shen added a comment - - edited

        Move the logic of above patch to Scan class, is it also OK?

        It means adding the ExplicitColumnsFilter in Scan.java when setting the attribute "SCAN-SMALL-ROWS"

        In addition, I worry about the data correctness if Scan already has complex filters.

        Show
        zjushch chunhui shen added a comment - - edited Move the logic of above patch to Scan class, is it also OK? It means adding the ExplicitColumnsFilter in Scan.java when setting the attribute "SCAN-SMALL-ROWS" In addition, I worry about the data correctness if Scan already has complex filters.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        In 0.94.12 the difference is not so dramatic as in 0.94.6 but still exists:

        default: 500K rows per sec
        filter-based: 1.2M rows per sec

        It seems that there is performance regression in scan filters in 0.94.12. The code which gives me almost 1.5M in 0.94.6 runs only 1.2M rows per sec in 0.94.12.

        Show
        vrodionov Vladimir Rodionov added a comment - In 0.94.12 the difference is not so dramatic as in 0.94.6 but still exists: default: 500K rows per sec filter-based: 1.2M rows per sec It seems that there is performance regression in scan filters in 0.94.12. The code which gives me almost 1.5M in 0.94.6 runs only 1.2M rows per sec in 0.94.12.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Also try with 0.94.12. The specific issue you're seeing might be fixed there.

        Show
        lhofhansl Lars Hofhansl added a comment - Also try with 0.94.12. The specific issue you're seeing might be fixed there.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Ted, is this suggestion to change the attribute name? Smallness of rows is not easy to estimate automatically that is why I suggest using explicit hint in a Scan instance.

        Show
        vrodionov Vladimir Rodionov added a comment - Ted, is this suggestion to change the attribute name? Smallness of rows is not easy to estimate automatically that is why I suggest using explicit hint in a Scan instance.
        Hide
        yuzhihong@gmail.com Ted Yu added a comment -

        Generally reseeks are better if they can skip many KVs.

        There is already a feature for small scans. If small in "SCAN-SMALL-ROWS" is replaced with narrow (or something similar), would it help clarify its purpose ?

        Show
        yuzhihong@gmail.com Ted Yu added a comment - Generally reseeks are better if they can skip many KVs. There is already a feature for small scans. If small in "SCAN-SMALL-ROWS" is replaced with narrow (or something similar), would it help clarify its purpose ?
        Hide
        vrodionov Vladimir Rodionov added a comment -

        The main idea is to provide new Scanner's hint (via new attribute) for RS - something SCAN-SMALL-ROWS and in Store.getScanners we will check for this attribute and if it is present we use StoreSCanner ctor with NULL as columns set:

         public KeyValueScanner getScanner(Scan scan,
              final NavigableSet<byte []> targetCols) throws IOException {
            lock.readLock().lock();
            boolean smallRowsScan = scan.getAttribute("SCAN-SMALL-ROWS") != null;
            if(smallRowsScan){
                  Filter ecFilter = new ExplicitColumnsFilter(targetCols);
                  // update filter in Scan with ecFilter
                  // remove  columnFamilyMap from Scan
            }
            try {
              KeyValueScanner scanner = null;
              if (getHRegion().getCoprocessorHost() != null) {
                scanner = getHRegion().getCoprocessorHost().preStoreScannerOpen(this, scan, smallRowsScan? null: targetCols);
              }
              if (scanner == null) {
                scanner = new StoreScanner(this, getScanInfo(), scan, smallRowsScan? null: targetCols:targetCols);
              }
              return scanner;
            } finally {
              lock.readLock().unlock();
            }
          }
        
        

        If no attribute than - default path

        Show
        vrodionov Vladimir Rodionov added a comment - The main idea is to provide new Scanner's hint (via new attribute) for RS - something SCAN-SMALL-ROWS and in Store.getScanners we will check for this attribute and if it is present we use StoreSCanner ctor with NULL as columns set: public KeyValueScanner getScanner(Scan scan, final NavigableSet< byte []> targetCols) throws IOException { lock.readLock().lock(); boolean smallRowsScan = scan.getAttribute( "SCAN-SMALL-ROWS" ) != null ; if (smallRowsScan){ Filter ecFilter = new ExplicitColumnsFilter(targetCols); // update filter in Scan with ecFilter // remove columnFamilyMap from Scan } try { KeyValueScanner scanner = null ; if (getHRegion().getCoprocessorHost() != null ) { scanner = getHRegion().getCoprocessorHost().preStoreScannerOpen( this , scan, smallRowsScan? null : targetCols); } if (scanner == null ) { scanner = new StoreScanner( this , getScanInfo(), scan, smallRowsScan? null : targetCols:targetCols); } return scanner; } finally { lock.readLock().unlock(); } } If no attribute than - default path
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Lars. I am going (finally) to create the patch. The code is running inside my test bed which is slightly off from any HBase revs and trunk.

        Show
        vrodionov Vladimir Rodionov added a comment - Lars. I am going (finally) to create the patch. The code is running inside my test bed which is slightly off from any HBase revs and trunk.
        Hide
        lhofhansl Lars Hofhansl added a comment -

        Show us the code

        Show
        lhofhansl Lars Hofhansl added a comment - Show us the code
        Hide
        vrodionov Vladimir Rodionov added a comment -

        Optimization described with initial results:

        Yes, I load data into HRegion (with CACHE_ON_WRITE) than call flashcache() (no data in memstore).
        
        This is what I found: the default implementation of  ExplicitColumnMatcher is (possibly) tuned to very large rows, I would say - very large. We need a hint for scan which  tells StoreScanner which strategy to use :
        
        1. ExplicitColumnMatcher with reseeks (what we have currently) for very large rows
        Or for small/medium rows
        2. Remove explicit columns/families  from a Scan and replace them with additional filter which actually keeps columnFamilyMap from scan and verifies every KV  matches with this map.
        
        I have created such a filter (ExplicitColumnsFilter) and verified that it works much better than case 1. for small/medium rows. For 1 CF + 5 CQs and Scan with 2 CQs I have:
        
        400K rows per sec with default
        1.45M with ExplicitScanReplacementFilter (> 350% improvement)
        
        Raw scanner (no columns specified) runs at 1.6M rows per sec. Its just 10% performance hit to run scanner with 2 explicit column qualifiers.
        
        Show
        vrodionov Vladimir Rodionov added a comment - Optimization described with initial results: Yes, I load data into HRegion (with CACHE_ON_WRITE) than call flashcache() (no data in memstore). This is what I found: the default implementation of ExplicitColumnMatcher is (possibly) tuned to very large rows, I would say - very large. We need a hint for scan which tells StoreScanner which strategy to use : 1. ExplicitColumnMatcher with reseeks (what we have currently) for very large rows Or for small/medium rows 2. Remove explicit columns/families from a Scan and replace them with additional filter which actually keeps columnFamilyMap from scan and verifies every KV matches with this map. I have created such a filter (ExplicitColumnsFilter) and verified that it works much better than case 1. for small/medium rows. For 1 CF + 5 CQs and Scan with 2 CQs I have: 400K rows per sec with default 1.45M with ExplicitScanReplacementFilter (> 350% improvement) Raw scanner (no columns specified) runs at 1.6M rows per sec. Its just 10% performance hit to run scanner with 2 explicit column qualifiers.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        LarsH:

        Interesting. Thanks for doing the testing/profiling Vladimir!
        
        Generally reseeks are better if they can skip many KVs.
        
        For example if you have many versions of the same row/col, INCLUDE_NEXT_COL will be better than issuing many INCLUDEs, same with INCLUDE_NEXT_ROW if there are many columns.
        
        Since the number of columns/versions is not known at scan time (and can in fact vary between rows) it is hard to always do the right thing. It also depends on how large the KVs are average. So replacing INCLUDE_NEXT_XXX with INCLUDE is not always the right idea.
        
        Thinking aloud... We could take the VERSIONS setting of the column family into account as a guideline for the expected number of versions (but there's no guarantee about how many version we'll actually have until we had a compaction), and replace INCLUDE_NEXT_COL with INCLUDE if VERSIONS is small (maybe < 10 or so). Maybe that'd be worth a jira...
        
        
        There are some fixes in 0.94.12 (HBASE-8930, avoid a superfluous reseek in some cases), and HBASE-9732 might help in 0.94.13 (avoid memory fences on an volatile on each seek/reseek).
        
        It also would be nice to figure out why reseek is so much more expensive. If the KV we reseek to is on the same block it should just scan forward, otherwise it'll look in the appropriate block. It probably is the creation of the fake KV we want to seek to (like firstOnRow, lastOnRow, etc), which case there's not much we can.
        
        Lastly, I've not spend much time profiling the ExplicitColumnMatcher, yet, looks like I should start doing that.
        
        
        Show
        vrodionov Vladimir Rodionov added a comment - LarsH: Interesting. Thanks for doing the testing/profiling Vladimir! Generally reseeks are better if they can skip many KVs. For example if you have many versions of the same row/col, INCLUDE_NEXT_COL will be better than issuing many INCLUDEs, same with INCLUDE_NEXT_ROW if there are many columns. Since the number of columns/versions is not known at scan time (and can in fact vary between rows) it is hard to always do the right thing. It also depends on how large the KVs are average. So replacing INCLUDE_NEXT_XXX with INCLUDE is not always the right idea. Thinking aloud... We could take the VERSIONS setting of the column family into account as a guideline for the expected number of versions (but there's no guarantee about how many version we'll actually have until we had a compaction), and replace INCLUDE_NEXT_COL with INCLUDE if VERSIONS is small (maybe < 10 or so). Maybe that'd be worth a jira... There are some fixes in 0.94.12 (HBASE-8930, avoid a superfluous reseek in some cases), and HBASE-9732 might help in 0.94.13 (avoid memory fences on an volatile on each seek/reseek). It also would be nice to figure out why reseek is so much more expensive. If the KV we reseek to is on the same block it should just scan forward, otherwise it'll look in the appropriate block. It probably is the creation of the fake KV we want to seek to (like firstOnRow, lastOnRow, etc), which case there's not much we can. Lastly, I've not spend much time profiling the ExplicitColumnMatcher, yet, looks like I should start doing that.
        Hide
        vrodionov Vladimir Rodionov added a comment -

        From dev-list:

        I modified tests:
        Now I created table with one CF and 5 columns: CQ1,..,CQ5
        1. Scan.addColumn(CF, CQ1);
            Scan.addColumn(CF, CQ3);
        2. Scan.addFamily(CF);
        Scan performance from block cache:
        1.  400K rows per sec
        2.  1.6M rows per sec
        The explicit columns scan performance  is even worse in this case. It is much faster to scan the WHOLE rows and filter columns later in a Filter, than specify columns directly in a Scan.
        

        I profiled the last test case (5 columns total and 2 in a scan).

        80% of StoreScanner.next() execution time are in :

        StoreScanner.reseek() - 71%
        ScanQueryMathcer.getKeyForNextColumn() - 6%
        ScanQueryMathcer.getKeyForNextRow() - 2%

        
        
        Show
        vrodionov Vladimir Rodionov added a comment - From dev-list: I modified tests: Now I created table with one CF and 5 columns: CQ1,..,CQ5 1. Scan.addColumn(CF, CQ1); Scan.addColumn(CF, CQ3); 2. Scan.addFamily(CF); Scan performance from block cache: 1. 400K rows per sec 2. 1.6M rows per sec The explicit columns scan performance is even worse in this case . It is much faster to scan the WHOLE rows and filter columns later in a Filter, than specify columns directly in a Scan. I profiled the last test case (5 columns total and 2 in a scan). 80% of StoreScanner.next() execution time are in : StoreScanner.reseek() - 71% ScanQueryMathcer.getKeyForNextColumn() - 6% ScanQueryMathcer.getKeyForNextRow() - 2%
        Hide
        vrodionov Vladimir Rodionov added a comment -

        From dev-hbase list:

        Simple table: one column + one qualifier
        Two type of scans:
        1. Scan.addFamily(CF)
        2. Scan.addColumn(CF, CQ)
        
        Both run on block cache (all data in memory)
        Tested on StoreScanner directly.
        1. 4.2M KVs per sec per one thread
        2. 1.5M KVs per second per one thread.
        The difference? First scanner's ScanQueryMatcher returns INCLUDE, DONE, second - INCLUDE_NEXT_ROW, DONE
        The cost of Row's reseek is huge.
        
        Show
        vrodionov Vladimir Rodionov added a comment - From dev-hbase list: Simple table: one column + one qualifier Two type of scans: 1. Scan.addFamily(CF) 2. Scan.addColumn(CF, CQ) Both run on block cache (all data in memory) Tested on StoreScanner directly. 1. 4.2M KVs per sec per one thread 2. 1.5M KVs per second per one thread. The difference? First scanner's ScanQueryMatcher returns INCLUDE, DONE, second - INCLUDE_NEXT_ROW, DONE The cost of Row's reseek is huge.

          People

          • Assignee:
            vrodionov Vladimir Rodionov
            Reporter:
            vrodionov Vladimir Rodionov
          • Votes:
            0 Vote for this issue
            Watchers:
            13 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development