HBase
  1. HBase
  2. HBASE-5925

Issue with only using the old config param hbase.hstore.compactionThreshold but not the corresponding new one

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      One observation while going through the code:-

      In MemStoreFlusher constructor

      this.blockingStoreFilesNumber =
            conf.getInt("hbase.hstore.blockingStoreFiles", 7);
          if (this.blockingStoreFilesNumber == -1) {
            this.blockingStoreFilesNumber = 1 +
              conf.getInt("hbase.hstore.compactionThreshold", 3);
          }
      

      Here as per the code if hbase.hstore.blockingStoreFiles is configured as -1, we are making this value to be 1+ min files to compact

      But here we read the old config item only!

      Here also we need to read the new config 1st and if not there then the old one.. Is this a miss?

      Like
      conf.getInt("hbase.hstore.compaction.min",
      conf.getInt("hbase.hstore.compactionThreshold", 3))

      1. HBASE-5925.patch
        0.8 kB
        Anoop Sam John

        Activity

        Hide
        stack added a comment -

        I sort of follow. I don't understand the bit where you say "But here we read the old config item only!'. Help me out Anoop.

        HBASE-3272 is what changed this config. Looking at the patch over there, will it help explaining why code is the way it is? (Maybe it doesn't!)

        Show
        stack added a comment - I sort of follow. I don't understand the bit where you say "But here we read the old config item only!'. Help me out Anoop. HBASE-3272 is what changed this config. Looking at the patch over there, will it help explaining why code is the way it is? (Maybe it doesn't!)
        Hide
        Anoop Sam John added a comment -

        @Stack

        I have gone through the patch for HBASE-3272

        -      conf.getInt("hbase.hstore.blockingStoreFiles", -1);
        +      conf.getInt("hbase.hstore.blockingStoreFiles", 7);
             if (this.blockingStoreFilesNumber == -1) {
               this.blockingStoreFilesNumber = 1 +
                 conf.getInt("hbase.hstore.compactionThreshold", 3);
        

        As per this change the default value for the hbase.hstore.blockingStoreFiles is been considered as 7. Previously it was like 1+ value for hbase.hstore.compactionThreshold
        [See the code below which checks if (this.blockingStoreFilesNumber == -1)]

        I think now after the patch for HBASE-3272 this if check may not be needed

        if (this.blockingStoreFilesNumber == -1) {
               this.blockingStoreFilesNumber = 1 +
                 conf.getInt("hbase.hstore.compactionThreshold", 3);
        

        I didnt know about this patch based change and when seen this piece of code with if check for -1, it looks like if explicitely -1 is configured, it will take the blockingStoreFilesNumber as 1+ value of hbase.hstore.compactionThreshold. I thought that this was intentionally given.. What this defect tells is as we later changed the config name for hbase.hstore.compactionThreshold to hbase.hstore.compaction.min, we need to consider that value here with precedence over the value for hbase.hstore.compactionThreshold

        Hope the desc is clear to you now... Any way now that is not needed.. What I think is that we can remove the code

        if (this.blockingStoreFilesNumber == -1) {
               this.blockingStoreFilesNumber = 1 +
                 conf.getInt("hbase.hstore.compactionThreshold", 3);
        

        which might confuse us.. What do you say Stack.

        Show
        Anoop Sam John added a comment - @Stack I have gone through the patch for HBASE-3272 - conf.getInt( "hbase.hstore.blockingStoreFiles" , -1); + conf.getInt( "hbase.hstore.blockingStoreFiles" , 7); if ( this .blockingStoreFilesNumber == -1) { this .blockingStoreFilesNumber = 1 + conf.getInt( "hbase.hstore.compactionThreshold" , 3); As per this change the default value for the hbase.hstore.blockingStoreFiles is been considered as 7. Previously it was like 1+ value for hbase.hstore.compactionThreshold [See the code below which checks if (this.blockingStoreFilesNumber == -1)] I think now after the patch for HBASE-3272 this if check may not be needed if ( this .blockingStoreFilesNumber == -1) { this .blockingStoreFilesNumber = 1 + conf.getInt( "hbase.hstore.compactionThreshold" , 3); I didnt know about this patch based change and when seen this piece of code with if check for -1, it looks like if explicitely -1 is configured, it will take the blockingStoreFilesNumber as 1+ value of hbase.hstore.compactionThreshold. I thought that this was intentionally given.. What this defect tells is as we later changed the config name for hbase.hstore.compactionThreshold to hbase.hstore.compaction.min, we need to consider that value here with precedence over the value for hbase.hstore.compactionThreshold Hope the desc is clear to you now... Any way now that is not needed.. What I think is that we can remove the code if ( this .blockingStoreFilesNumber == -1) { this .blockingStoreFilesNumber = 1 + conf.getInt( "hbase.hstore.compactionThreshold" , 3); which might confuse us.. What do you say Stack.
        Hide
        stack added a comment -

        If you make a patch, I'll apply it.

        Show
        stack added a comment - If you make a patch, I'll apply it.
        Hide
        Anoop Sam John added a comment -

        Simple patch for trunk

        Show
        Anoop Sam John added a comment - Simple patch for trunk
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12526008/HBASE-5925.patch
        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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1799//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1799//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1799//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/12526008/HBASE-5925.patch 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 hadoop23. The patch compiles against the hadoop 0.23.x 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 core tests. The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1799//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1799//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1799//console This message is automatically generated.
        Hide
        stack added a comment -

        Committed trunk. Thanks Anoop.

        Show
        stack added a comment - Committed trunk. Thanks Anoop.
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2857 (See https://builds.apache.org/job/HBase-TRUNK/2857/)
        HBASE-5925 Issue with only using the old config param hbase.hstore.compactionThreshold but not the corresponding new one (Revision 1335738)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2857 (See https://builds.apache.org/job/HBase-TRUNK/2857/ ) HBASE-5925 Issue with only using the old config param hbase.hstore.compactionThreshold but not the corresponding new one (Revision 1335738) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK-security #196 (See https://builds.apache.org/job/HBase-TRUNK-security/196/)
        HBASE-5925 Issue with only using the old config param hbase.hstore.compactionThreshold but not the corresponding new one (Revision 1335738)

        Result = FAILURE
        stack :
        Files :

        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK-security #196 (See https://builds.apache.org/job/HBase-TRUNK-security/196/ ) HBASE-5925 Issue with only using the old config param hbase.hstore.compactionThreshold but not the corresponding new one (Revision 1335738) Result = FAILURE stack : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreFlusher.java
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.

          People

          • Assignee:
            Anoop Sam John
            Reporter:
            Anoop Sam John
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development