Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: None
    • Labels:
      None

      Description

      In ScanWildcardColumnTracker we have

       
        this.oldestStamp = EnvironmentEdgeManager.currentTimeMillis() - ttl;
      
        ...
      
        private boolean isExpired(long timestamp) {
          return timestamp < oldestStamp;
        }
      

      but this time range filtering does not participate in HFile selection. In one real case this caused next() calls to time out because all KVs in a table got expired, but next() had to iterate over the whole table to find that out. We should be able to filter out those HFiles right away. I think a reasonable approach is to add a "default timerange filter" to every scan for a CF with a finite TTL and utilize existing filtering in StoreFile.Reader.passesTimerangeFilter.

      1. ASF.LICENSE.NOT.GRANTED--D909.1.patch
        31 kB
        Phabricator
      2. ASF.LICENSE.NOT.GRANTED--D909.2.patch
        32 kB
        Phabricator
      3. ASF.LICENSE.NOT.GRANTED--D1017.1.patch
        42 kB
        Phabricator
      4. ASF.LICENSE.NOT.GRANTED--D1017.2.patch
        42 kB
        Phabricator
      5. 5010.patch
        44 kB
        Ted Yu
      6. ASF.LICENSE.NOT.GRANTED--D909.3.patch
        32 kB
        Phabricator
      7. ASF.LICENSE.NOT.GRANTED--D909.4.patch
        35 kB
        Phabricator
      8. ASF.LICENSE.NOT.GRANTED--D909.5.patch
        37 kB
        Phabricator
      9. ASF.LICENSE.NOT.GRANTED--D909.6.patch
        40 kB
        Phabricator

        Issue Links

          Activity

          Hide
          Mikhail Bautin added a comment -

          Actually here is the reason for those confusing updates. Ted seems to have specified HBASE-5010 instead of HBASE-5510 in the commit message.

          commit 5d773d9fa176cb056b993fdff8a2853f75315ec8
          Author: tedyu <tedyu@13f79535-47bb-0310-9956-ffa450edef68>
          Date: Mon Mar 5 10:41:03 2012

          HBASE-5010 Pass region info in LoadBalancer.randomAssignment(List<ServerName> servers) (Anoop Sam

          git-svn-id: http://svn.apache.org/repos/asf/hbase/trunk@1297155 13f79535-47bb-0310-9956-ffa450edef68

          Show
          Mikhail Bautin added a comment - Actually here is the reason for those confusing updates. Ted seems to have specified HBASE-5010 instead of HBASE-5510 in the commit message. commit 5d773d9fa176cb056b993fdff8a2853f75315ec8 Author: tedyu <tedyu@13f79535-47bb-0310-9956-ffa450edef68> Date: Mon Mar 5 10:41:03 2012 HBASE-5010 Pass region info in LoadBalancer.randomAssignment(List<ServerName> servers) (Anoop Sam git-svn-id: http://svn.apache.org/repos/asf/hbase/trunk@1297155 13f79535-47bb-0310-9956-ffa450edef68
          Hide
          ramkrishna.s.vasudevan added a comment -

          @Mikhail
          The commit related updates that usually comes up once a commit is done was appearing in this JIRA. But it was for HBASE-5510. Ted removed them as it was not related to this JIRA. May be that deleted part you are not able to view now.
          Sorry if the above comment had confused you.

          Show
          ramkrishna.s.vasudevan added a comment - @Mikhail The commit related updates that usually comes up once a commit is done was appearing in this JIRA. But it was for HBASE-5510 . Ted removed them as it was not related to this JIRA. May be that deleted part you are not able to view now. Sorry if the above comment had confused you.
          Hide
          Mikhail Bautin added a comment -

          @Ram: I don't see any mentions of HBASE-5510 in this JIRA, except for your comment. What updates are you referring to?

          Show
          Mikhail Bautin added a comment - @Ram: I don't see any mentions of HBASE-5510 in this JIRA, except for your comment. What updates are you referring to?
          Hide
          Ramkrishna.S.Vasudevan added a comment -

          Why HBASE-5510 updates coming in this JIRA?

          Regards
          Ram

          Show
          Ramkrishna.S.Vasudevan added a comment - Why HBASE-5510 updates coming in this JIRA? Regards Ram
          Hide
          Lars Hofhansl added a comment -

          Thanks for clarifying Prakash.

          Show
          Lars Hofhansl added a comment - Thanks for clarifying Prakash.
          Hide
          Prakash Khemani added a comment -

          This change is doesn't break HBASE-4721.

          HBASE-4721 introduced another parameter called hbase.hstore.time.to.purge.deletes to keep deletes even after major compactions. But hbase.hstore.time.to.purge.deletes doesn't override the TTL of the store.

          Pasting the comment from code which hopefully makes it clear that this diff works with HBASE-4721

          // By default, when hbase.hstore.time.to.purge.deletes is 0ms, a delete
          // marker is always removed during a major compaction. If set to non-zero
          // value then major compaction will try to keep a delete marker around for
          // the given number of milliseconds. We want to keep the delete markers
          // around a bit longer because old puts might appear out-of-order. For
          // example, during log replication between two clusters.
          //
          // If the delete marker has lived longer than its column-family's TTL then
          // the delete marker will be removed even if time.to.purge.deletes has not
          // passed. This is because all the Puts that this delete marker can influence
          // would have also expired. (Removing of delete markers on col family TTL will
          // not happen if min-versions is set to non-zero)
          //

          Show
          Prakash Khemani added a comment - This change is doesn't break HBASE-4721 . HBASE-4721 introduced another parameter called hbase.hstore.time.to.purge.deletes to keep deletes even after major compactions. But hbase.hstore.time.to.purge.deletes doesn't override the TTL of the store. Pasting the comment from code which hopefully makes it clear that this diff works with HBASE-4721 // By default, when hbase.hstore.time.to.purge.deletes is 0ms, a delete // marker is always removed during a major compaction. If set to non-zero // value then major compaction will try to keep a delete marker around for // the given number of milliseconds. We want to keep the delete markers // around a bit longer because old puts might appear out-of-order. For // example, during log replication between two clusters. // // If the delete marker has lived longer than its column-family's TTL then // the delete marker will be removed even if time.to.purge.deletes has not // passed. This is because all the Puts that this delete marker can influence // would have also expired. (Removing of delete markers on col family TTL will // not happen if min-versions is set to non-zero) //
          Hide
          Mikhail Bautin added a comment -

          A follow-up fix was submitted as part of HBASE-5274 to bring the trunk fix for this issue to parity with the 89-fb fix. Resolving.

          Show
          Mikhail Bautin added a comment - A follow-up fix was submitted as part of HBASE-5274 to bring the trunk fix for this issue to parity with the 89-fb fix. Resolving.
          Hide
          Mikhail Bautin added a comment -

          @Stack: there a small item pending here. The 89-fb patch contains more testing for the compaction case, which has to be ported to trunk.

          @Lars: I will work with Prakash to ensure this does not break HBASE-4721. I believe HBASE-4721 is only used for a replication approach that is still in development.

          I think we'll keep this JIRA open until the two above items are resolved, unless anyone objects.

          Show
          Mikhail Bautin added a comment - @Stack: there a small item pending here. The 89-fb patch contains more testing for the compaction case, which has to be ported to trunk. @Lars: I will work with Prakash to ensure this does not break HBASE-4721 . I believe HBASE-4721 is only used for a replication approach that is still in development. I think we'll keep this JIRA open until the two above items are resolved, unless anyone objects.
          Hide
          Phabricator added a comment -

          nspiegelberg has committed the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          REVISION DETAIL
          https://reviews.facebook.net/D909

          COMMIT
          https://reviews.facebook.net/rHBASEEIGHTNINEFBBRANCH1232732

          Show
          Phabricator added a comment - nspiegelberg has committed the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". REVISION DETAIL https://reviews.facebook.net/D909 COMMIT https://reviews.facebook.net/rHBASEEIGHTNINEFBBRANCH1232732
          Hide
          Lars Hofhansl added a comment -

          Actually... Does this work together with HBASE-4721 correctly?
          HBASE-4721 allows keeping delete markers longer than the TTL set for the store.

          Show
          Lars Hofhansl added a comment - Actually... Does this work together with HBASE-4721 correctly? HBASE-4721 allows keeping delete markers longer than the TTL set for the store.
          Hide
          stack added a comment -

          Can we close this? Its been applied to trunk. Its not in fb-0.89?

          Show
          stack added a comment - Can we close this? Its been applied to trunk. Its not in fb-0.89?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12508883/D909.6.patch
          against trunk revision .

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/633//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/12508883/D909.6.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 20 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/633//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          Kannan has accepted the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          awesome optimization and nice work!

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - Kannan has accepted the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". awesome optimization and nice work! REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, JIRA

          Addressing the issue that Kannan pointed out: getScanner does not use its arguments. It turned out that getScanner was called with this.scan and this.columns in all callsites, so I have removed those arguments.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Reviewers: Kannan, Liyin, JIRA Addressing the issue that Kannan pointed out: getScanner does not use its arguments. It turned out that getScanner was called with this.scan and this.columns in all callsites, so I have removed those arguments. REVISION DETAIL https://reviews.facebook.net/D909 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/Store.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          Hide
          Phabricator added a comment -

          mbautin has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:209 Yes, that's correct.
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:224 Yes, this is the way it was in the original fix (89-fb version).

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:209 Yes, that's correct. src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:224 Yes, this is the way it was in the original fix (89-fb version). REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          Kannan has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:209 scan & columns arguments no longer used?
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:224 and we are using this.scan & this.columns here?

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:209 scan & columns arguments no longer used? src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:224 and we are using this.scan & this.columns here? REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          mbautin has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:155 This is why we filter out expired StoreFiles on compactions.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:155 This is why we filter out expired StoreFiles on compactions. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12508817/D909.5.patch
          against trunk revision .

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/620//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/12508817/D909.5.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 20 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/620//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, JIRA

          Actually, this is where I addressed Kannan's comment about compactions:

          https://reviews.facebook.net/D909?vs=2679&id=3015&whitespace=ignore-all

          (see the new line 154 in StoreScanner.java:

          scanners = selectScannersFrom(scanners);

          )

          I have spent quite a bit of time making sure that the unit test is testing the optimization during compactions. I am now invoking compactions in two different ways, for a total of 6 parameterized instances of the test, but it is still really quick. All of our compaction codepaths go through StoreScanner constructors, so we should have the optimization in all of them.

          All unit tests pass.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Reviewers: Kannan, Liyin, JIRA Actually, this is where I addressed Kannan's comment about compactions: https://reviews.facebook.net/D909?vs=2679&id=3015&whitespace=ignore-all (see the new line 154 in StoreScanner.java: scanners = selectScannersFrom(scanners); ) I have spent quite a bit of time making sure that the unit test is testing the optimization during compactions. I am now invoking compactions in two different ways, for a total of 6 parameterized instances of the test, but it is still really quick. All of our compaction codepaths go through StoreScanner constructors, so we should have the optimization in all of them. All unit tests pass. REVISION DETAIL https://reviews.facebook.net/D909 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/Store.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12508811/D909.4.patch
          against trunk revision .

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/619//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/12508811/D909.4.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 17 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/619//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          mbautin has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:154 This is addressed in the most recent version of the diff.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:154 This is addressed in the most recent version of the diff. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          mbautin has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:219 Assuming that a default scan will go through all KVs. Please let me know if this is incorrect.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:219 Assuming that a default scan will go through all KVs. Please let me know if this is incorrect. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, JIRA

          Addressing Kannan's comment about doing the same optimization during compactions. Adding a compaction test to the unit test, and verifying that we don't read expired files using per-CF metrics.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Reviewers: Kannan, Liyin, JIRA Addressing Kannan's comment about doing the same optimization during compactions. Adding a compaction test to the unit test, and verifying that we don't read expired files using per-CF metrics. REVISION DETAIL https://reviews.facebook.net/D909 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/Store.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          Hide
          Phabricator added a comment -

          Kannan has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          The compaction code path doesn't yet get the benefit of this optimization. Spoke with Mikhail offline, and he'll update the diff to handle this case.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". The compaction code path doesn't yet get the benefit of this optimization. Spoke with Mikhail offline, and he'll update the diff to handle this case. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-security #49 (See https://builds.apache.org/job/HBase-TRUNK-security/49/)
          HBASE-5010 Filter HFiles based on TTL - add TestScannerSelectionUsingTTL
          HBASE-5010 Filter HFiles based on TTL (Mikhail)

          tedyu :
          Files :

          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java

          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-security #49 (See https://builds.apache.org/job/HBase-TRUNK-security/49/ ) HBASE-5010 Filter HFiles based on TTL - add TestScannerSelectionUsingTTL HBASE-5010 Filter HFiles based on TTL (Mikhail) tedyu : Files : /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
          Hide
          Phabricator added a comment -

          mbautin has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          See some replies to comments inline.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:133 Removed the line about major compactions.
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:149 Fixed.
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java:80 Fixed.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". See some replies to comments inline. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:133 Removed the line about major compactions. src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:149 Fixed. src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java:80 Fixed. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          mbautin has abandoned the revision "[jira] HBASE-5010 Filter HFiles based on TTL".

          This was committed into trunk by @tedyu, closing the revision.

          @tedyu: could you please include a line of the form "Differential Revision: D1017" in commit messages in the future? That way the revision could be automatically marked as "committed" in Phabricator.

          REVISION DETAIL
          https://reviews.facebook.net/D1017

          Show
          Phabricator added a comment - mbautin has abandoned the revision " [jira] HBASE-5010 Filter HFiles based on TTL". This was committed into trunk by @tedyu, closing the revision. @tedyu: could you please include a line of the form "Differential Revision: D1017" in commit messages in the future? That way the revision could be automatically marked as "committed" in Phabricator. REVISION DETAIL https://reviews.facebook.net/D1017
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12508643/D909.3.patch
          against trunk revision .

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/603//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/12508643/D909.3.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 17 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/603//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, JIRA

          Addressing Lars's comment.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Reviewers: Kannan, Liyin, JIRA Addressing Lars's comment. REVISION DETAIL https://reviews.facebook.net/D909 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          Hide
          Phabricator added a comment -

          mbautin has commented on the revision "[jira] HBASE-5010 Filter HFiles based on TTL".

          Lars: replying to your comments inline. I started addressing them but Ted addressed some of them and committed the patch first Not sure if anything was done about emphasizing the incompatibility of this fix with MIN_VERSIONS, though. At least, this fix does not make things worse if MIN_VERSIONS is nonzero. A performance penalty could only be incurred when there are entire HFiles consisting of expired entries and MIN_VERSIONS is nonzero.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:737 Done.
          src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java:74 Done.
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java:134 Done.

          REVISION DETAIL
          https://reviews.facebook.net/D1017

          Show
          Phabricator added a comment - mbautin has commented on the revision " [jira] HBASE-5010 Filter HFiles based on TTL". Lars: replying to your comments inline. I started addressing them but Ted addressed some of them and committed the patch first Not sure if anything was done about emphasizing the incompatibility of this fix with MIN_VERSIONS, though. At least, this fix does not make things worse if MIN_VERSIONS is nonzero. A performance penalty could only be incurred when there are entire HFiles consisting of expired entries and MIN_VERSIONS is nonzero. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:737 Done. src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java:74 Done. src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java:134 Done. REVISION DETAIL https://reviews.facebook.net/D1017
          Hide
          Ted Yu added a comment -

          The exception was Due to mapreduce-3583

          Show
          Ted Yu added a comment - The exception was Due to mapreduce-3583
          Hide
          Lars Hofhansl added a comment -

          We have a bunch of other optimizations in 0.94 only. I think this should be 0.94 only as well.

          Show
          Lars Hofhansl added a comment - We have a bunch of other optimizations in 0.94 only. I think this should be 0.94 only as well.
          Hide
          Mikhail Bautin added a comment -

          @Ted: thanks for taking care of the patch! A couple of questions:
          (1) Do you know why these spurious NumberFormatExceptions occur? Has this happened before?
          (2) Do you think we need this optimization in 0.92?

          @Lars: thanks for the review!

          Show
          Mikhail Bautin added a comment - @Ted: thanks for taking care of the patch! A couple of questions: (1) Do you know why these spurious NumberFormatExceptions occur? Has this happened before? (2) Do you think we need this optimization in 0.92? @Lars: thanks for the review!
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2579 (See https://builds.apache.org/job/HBase-TRUNK/2579/)
          HBASE-5010 Filter HFiles based on TTL - add TestScannerSelectionUsingTTL
          HBASE-5010 Filter HFiles based on TTL (Mikhail)

          tedyu :
          Files :

          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java

          tedyu :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2579 (See https://builds.apache.org/job/HBase-TRUNK/2579/ ) HBASE-5010 Filter HFiles based on TTL - add TestScannerSelectionUsingTTL HBASE-5010 Filter HFiles based on TTL (Mikhail) tedyu : Files : /hbase/trunk/src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java tedyu : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
          Hide
          Ted Yu added a comment -

          Test failures reported by Hadoop QA were due to NumberFormatException

          Patch integrated to TRUNK.

          Thanks for the patch Mikhail.

          Thanks for the review Lars.

          I tried applying 5010.patch to 0.92 but got some rejected changes.

          Show
          Ted Yu added a comment - Test failures reported by Hadoop QA were due to NumberFormatException Patch integrated to TRUNK. Thanks for the patch Mikhail. Thanks for the review Lars. I tried applying 5010.patch to 0.92 but got some rejected changes.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12508631/5010.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -151 warning messages.

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

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

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

          -1 core tests. The patch failed these unit tests:
          org.apache.hadoop.hbase.mapred.TestTableMapReduce
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/601//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/601//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/601//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/12508631/5010.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 30 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 77 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.mapred.TestTableMapReduce org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/601//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/601//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/601//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Fix compilation error in TestScannerSelectionUsingTTL

          Show
          Ted Yu added a comment - Fix compilation error in TestScannerSelectionUsingTTL
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12508628/5010.patch
          against trunk revision .

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

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

          -1 javadoc. The javadoc tool appears to have generated -151 warning messages.

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

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

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

          -1 core tests. The patch failed these unit tests:

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/600//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/600//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/600//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/12508628/5010.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 30 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -151 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 77 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/600//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/600//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/600//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Moved the new test to src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java

          Added javadoc for oldestUnexpiredTS

          Added test category for TestScannerSelectionUsingTTL

          Show
          Ted Yu added a comment - Moved the new test to src/test/java/org/apache/hadoop/hbase/io/hfile/TestScannerSelectionUsingTTL.java Added javadoc for oldestUnexpiredTS Added test category for TestScannerSelectionUsingTTL
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-5010 Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, tedyu, JIRA

          Addressing Ted's comment and getting rid of unused ttl parameters in a couple of places.

          REVISION DETAIL
          https://reviews.facebook.net/D1017

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5010 Filter HFiles based on TTL". Reviewers: Kannan, Liyin, tedyu, JIRA Addressing Ted's comment and getting rid of unused ttl parameters in a couple of places. REVISION DETAIL https://reviews.facebook.net/D1017 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java src/main/java/org/apache/hadoop/hbase/regionserver/Store.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "[jira] HBASE-5010 Filter HFiles based on TTL".

          Looks pretty good. See minor comments inline.
          minVersions handling looks correct to me.

          I think we should call out somewhere that this optimization cannot be used with MIN_VERSIONS enabled, along with some guess for the typical performance penalty.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:737 As said in my 0.90 review, this is better package private with the test class in the same package.
          src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java:74 Javadoc for oldestUnexpiredTS is missing.
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java:134 Javadoc for oldestUnexpiredTS

          REVISION DETAIL
          https://reviews.facebook.net/D1017

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " [jira] HBASE-5010 Filter HFiles based on TTL". Looks pretty good. See minor comments inline. minVersions handling looks correct to me. I think we should call out somewhere that this optimization cannot be used with MIN_VERSIONS enabled, along with some guess for the typical performance penalty. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:737 As said in my 0.90 review, this is better package private with the test class in the same package. src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java:74 Javadoc for oldestUnexpiredTS is missing. src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java:134 Javadoc for oldestUnexpiredTS REVISION DETAIL https://reviews.facebook.net/D1017
          Hide
          Lars Hofhansl added a comment -

          MinVersions is my doing. I'll take a look as soon as I get a chance.

          Show
          Lars Hofhansl added a comment - MinVersions is my doing. I'll take a look as soon as I get a chance.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12508613/D1017.1.patch
          against trunk revision .

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/598//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/12508613/D1017.1.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 29 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/598//console This message is automatically generated.
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "[jira] HBASE-5010 Filter HFiles based on TTL".

          Looks good.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java:1207 Should we mention oldestUnexpiredTS here ?

          REVISION DETAIL
          https://reviews.facebook.net/D1017

          Show
          Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-5010 Filter HFiles based on TTL". Looks good. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java:1207 Should we mention oldestUnexpiredTS here ? REVISION DETAIL https://reviews.facebook.net/D1017
          Hide
          Phabricator added a comment -

          mbautin requested code review of "[jira] HBASE-5010 Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, tedyu, JIRA

          This is the trunk version of D909. The main difference is that there is a minVersions CF setting in trunk, and when minVersions is not zero, we can't exclude StoreFiles based on TTL, because we might have to retrieve KVs with expired timestamps to comply with the minVersions requirement.

          TEST PLAN
          Unit tests (including a new one).

          REVISION DETAIL
          https://reviews.facebook.net/D1017

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java

          MANAGE HERALD DIFFERENTIAL RULES
          https://reviews.facebook.net/herald/view/differential/

          WHY DID I GET THIS EMAIL?
          https://reviews.facebook.net/herald/transcript/2127/

          Tip: use the X-Herald-Rules header to filter Herald messages in your client.

          Show
          Phabricator added a comment - mbautin requested code review of " [jira] HBASE-5010 Filter HFiles based on TTL". Reviewers: Kannan, Liyin, tedyu, JIRA This is the trunk version of D909. The main difference is that there is a minVersions CF setting in trunk, and when minVersions is not zero, we can't exclude StoreFiles based on TTL, because we might have to retrieve KVs with expired timestamps to comply with the minVersions requirement. TEST PLAN Unit tests (including a new one). REVISION DETAIL https://reviews.facebook.net/D1017 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/ExplicitColumnTracker.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanWildcardColumnTracker.java src/main/java/org/apache/hadoop/hbase/regionserver/Store.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMinVersions.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestScanWildcardColumnTracker.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/2127/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.
          Hide
          Phabricator added a comment -

          lhofhansl has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          lgtm
          See minor comment inline.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:832 Rather than making this public, should locate the TestClass in same package and this "package private". That would reduce the change of anybody accidentally using it.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - lhofhansl has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". lgtm See minor comment inline. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java:832 Rather than making this public, should locate the TestClass in same package and this "package private". That would reduce the change of anybody accidentally using it. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          mbautin updated the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, JIRA

          Addressing Kannan's and Ted's comments.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java

          Show
          Phabricator added a comment - mbautin updated the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Reviewers: Kannan, Liyin, JIRA Addressing Kannan's and Ted's comments. REVISION DETAIL https://reviews.facebook.net/D909 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java
          Hide
          Phabricator added a comment -

          tedyu has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          Nice work.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java:80 oldestUnexpiredTS is missing after @param
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:149 This change makes the line over 80 chars.
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:169 Line too long.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - tedyu has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Nice work. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java:80 oldestUnexpiredTS is missing after @param src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:149 This change makes the line over 80 chars. src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:169 Line too long. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          Kannan has commented on the revision "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".

          Mikhail: Nice work, and unit test. Compaction related comment inline.

          INLINE COMMENTS
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:133 pre-existing issue: this constructor is no longer just for major compactions. Minor compactions also use this.
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:154 If we do a similar check in this constructor, we would get the same optimization for compactions also.

          As we talked about offline, if the entire file has expired data, then we can avoid adding the scanner to the KeyValueHeap below. So for CFs which have routinely expiring data due to TTL, compactions would have to read a lot less data too or could essentially turn into feather-weight ops which just delete unnecessary/old files.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          Show
          Phabricator added a comment - Kannan has commented on the revision " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Mikhail: Nice work, and unit test. Compaction related comment inline. INLINE COMMENTS src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:133 pre-existing issue: this constructor is no longer just for major compactions. Minor compactions also use this. src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java:154 If we do a similar check in this constructor, we would get the same optimization for compactions also. As we talked about offline, if the entire file has expired data, then we can avoid adding the scanner to the KeyValueHeap below. So for CFs which have routinely expiring data due to TTL, compactions would have to read a lot less data too or could essentially turn into feather-weight ops which just delete unnecessary/old files. REVISION DETAIL https://reviews.facebook.net/D909
          Hide
          Phabricator added a comment -

          mbautin requested code review of "[jira] HBASE-5010 [89-fb] Filter HFiles based on TTL".
          Reviewers: Kannan, Liyin, JIRA

          Modifying scanner selection in StoreScanner to take TTL into account, so that we don't scan StoreFiles that only contain expired keys.

          This diff is for 89-fb, but there will be a very similar diff for trunk.

          TEST PLAN
          Unit tests (existing ones and a new one).
          Deploy to cluster and scan an existing table that only contains expired keys. Verify that next() calls don't time out. Previously, the scanner would try to go through the whole table in an attempt to find a non-expired key.

          REVISION DETAIL
          https://reviews.facebook.net/D909

          AFFECTED FILES
          src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java
          src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java
          src/main/java/org/apache/hadoop/hbase/util/Threads.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
          src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java

          MANAGE HERALD DIFFERENTIAL RULES
          https://reviews.facebook.net/herald/view/differential/

          WHY DID I GET THIS EMAIL?
          https://reviews.facebook.net/herald/transcript/1911/

          Tip: use the X-Herald-Rules header to filter Herald messages in your client.

          Show
          Phabricator added a comment - mbautin requested code review of " [jira] HBASE-5010 [89-fb] Filter HFiles based on TTL". Reviewers: Kannan, Liyin, JIRA Modifying scanner selection in StoreScanner to take TTL into account, so that we don't scan StoreFiles that only contain expired keys. This diff is for 89-fb, but there will be a very similar diff for trunk. TEST PLAN Unit tests (existing ones and a new one). Deploy to cluster and scan an existing table that only contains expired keys. Verify that next() calls don't time out. Previously, the scanner would try to go through the whole table in an attempt to find a non-expired key. REVISION DETAIL https://reviews.facebook.net/D909 AFFECTED FILES src/main/java/org/apache/hadoop/hbase/io/hfile/LruBlockCache.java src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java src/main/java/org/apache/hadoop/hbase/regionserver/NonLazyKeyValueScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java src/main/java/org/apache/hadoop/hbase/util/Threads.java src/test/java/org/apache/hadoop/hbase/regionserver/TestCompoundBloomFilter.java src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java src/test/java/org/apache/hadoop/hbase/regionserver/TestSelectScannersUsingTTL.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/1911/ Tip: use the X-Herald-Rules header to filter Herald messages in your client.

            People

            • Assignee:
              Mikhail Bautin
              Reporter:
              Mikhail Bautin
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development