HBase
  1. HBase
  2. HBASE-5720

HFileDataBlockEncoderImpl uses wrong header size when reading HFiles with no checksums

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.94.0
    • Fix Version/s: 0.94.0
    • Component/s: io, regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When reading a .92 HFile without checksums, encoding it, and storing in the block cache, the HFileDataBlockEncoderImpl always allocates a dummy header appropriate for checksums even though there are none. This corrupts the byte[].

      Attaching a patch that allocates a DUMMY_HEADER_NO_CHECKSUM in that case which I think is the desired behavior.

      1. HBASE-5720-v1.patch
        2 kB
        Matt Corgan
      2. HBASE-5720-v2.patch
        3 kB
        Matt Corgan
      3. HBASE-5720-v3.patch
        4 kB
        Matt Corgan
      4. 5720v4.txt
        3 kB
        stack
      5. 5720v4.txt
        3 kB
        stack
      6. 5720v4.txt
        3 kB
        Lars Hofhansl
      7. 5720-trunk.txt
        13 kB
        Ted Yu
      8. 5720-trunk-v2.txt
        14 kB
        Ted Yu

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in HBase-0.94-security #9 (See https://builds.apache.org/job/HBase-0.94-security/9/)
          HBASE-5720 HFileDataBlockEncoderImpl uses wrong header size when reading HFiles with no checksums (Matt Corgan) (Revision 1310932)

          Result = SUCCESS
          larsh :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
          Show
          Hudson added a comment - Integrated in HBase-0.94-security #9 (See https://builds.apache.org/job/HBase-0.94-security/9/ ) HBASE-5720 HFileDataBlockEncoderImpl uses wrong header size when reading HFiles with no checksums (Matt Corgan) (Revision 1310932) Result = SUCCESS larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
          Hide
          Hudson added a comment -

          Integrated in HBase-0.94 #98 (See https://builds.apache.org/job/HBase-0.94/98/)
          HBASE-5720 HFileDataBlockEncoderImpl uses wrong header size when reading HFiles with no checksums (Matt Corgan) (Revision 1310932)

          Result = SUCCESS
          larsh :
          Files :

          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
          • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
          Show
          Hudson added a comment - Integrated in HBase-0.94 #98 (See https://builds.apache.org/job/HBase-0.94/98/ ) HBASE-5720 HFileDataBlockEncoderImpl uses wrong header size when reading HFiles with no checksums (Matt Corgan) (Revision 1310932) Result = SUCCESS larsh : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java
          Hide
          Lars Hofhansl added a comment -

          Let's move all followup discussions into HBASE-5746

          Show
          Lars Hofhansl added a comment - Let's move all followup discussions into HBASE-5746
          Hide
          Lars Hofhansl added a comment -

          Comitted to 0.94 only.

          Show
          Lars Hofhansl added a comment - Comitted to 0.94 only.
          Hide
          Lars Hofhansl added a comment - - edited

          Added you as contributor Matt.

          Show
          Lars Hofhansl added a comment - - edited Added you as contributor Matt.
          Hide
          Matt Corgan added a comment -

          @Stack - yep. HBASE-4336 is pretty much what i'm getting at, but I don't know much about maven. I think one-shot modularization of a large, fast moving project is dangerous/impossible. A more practical approach is to decide on a couple modules and slowly start pulling things into them. It could take years and never really be finished, but every time something is extracted to a module the main project becomes a little bit simpler.

          Show
          Matt Corgan added a comment - @Stack - yep. HBASE-4336 is pretty much what i'm getting at, but I don't know much about maven. I think one-shot modularization of a large, fast moving project is dangerous/impossible. A more practical approach is to decide on a couple modules and slowly start pulling things into them. It could take years and never really be finished, but every time something is extracted to a module the main project becomes a little bit simpler.
          Hide
          Lars Hofhansl added a comment -

          @Matt... I'm an idiot, I used the wrong patch. Sigh.

          Show
          Lars Hofhansl added a comment - @Matt... I'm an idiot, I used the wrong patch. Sigh.
          Hide
          stack added a comment -

          @Matt I think you are making a new argument, number #4 I believe, for why we should do HBASE-4336.

          Show
          stack added a comment - @Matt I think you are making a new argument, number #4 I believe, for why we should do HBASE-4336 .
          Hide
          Matt Corgan added a comment -

          The ideal way to split code in my opinion would be to put it in multiple source directories in the same project, and then configure the build path to set up one-way visibility between source directories. For example, the src/test/java folder would be able to see the src/main/java folder but not vice versa. Unfortunately, eclipse does not support that. It has a global classpath per project. Can't find the exact discussion, but similar to: https://bugs.eclipse.org/bugs/show_bug.cgi?id=224708.

          I can think of two alternatives:

          1) Put multiple src directories in the main project, so everything is in that project's classpath. Periodically have a human or something else remove non-core src directories from the classpath and see if that causes compilation errors in the main src. I just tried removing the src/main/test directory from the classpath and was happy to see that the src/main/java folder still compiles.

          2) A more heavy handed approach would be to create separate projects so you can actually enforce unidirectional classpath visibility. That's how I've been developing HBASE-4676 which can be added to the main project as a separate jar. Main project still compiles without it. The downside of this is that having multiple projects is more configuration overhead, like you would probably need multiple svn repos.

          I think moving towards #1 is a good start for core stuff while more exotic contributions could be made by submitting jars. I'll try to nudge us in this direction with HBASE-4676, just wanted to make sure people agree that it's important =)

          Show
          Matt Corgan added a comment - The ideal way to split code in my opinion would be to put it in multiple source directories in the same project, and then configure the build path to set up one-way visibility between source directories. For example, the src/test/java folder would be able to see the src/main/java folder but not vice versa. Unfortunately, eclipse does not support that. It has a global classpath per project. Can't find the exact discussion, but similar to: https://bugs.eclipse.org/bugs/show_bug.cgi?id=224708 . I can think of two alternatives: 1) Put multiple src directories in the main project, so everything is in that project's classpath. Periodically have a human or something else remove non-core src directories from the classpath and see if that causes compilation errors in the main src. I just tried removing the src/main/test directory from the classpath and was happy to see that the src/main/java folder still compiles. 2) A more heavy handed approach would be to create separate projects so you can actually enforce unidirectional classpath visibility. That's how I've been developing HBASE-4676 which can be added to the main project as a separate jar. Main project still compiles without it. The downside of this is that having multiple projects is more configuration overhead, like you would probably need multiple svn repos. I think moving towards #1 is a good start for core stuff while more exotic contributions could be made by submitting jars. I'll try to nudge us in this direction with HBASE-4676 , just wanted to make sure people agree that it's important =)
          Hide
          Matt Corgan added a comment -

          Lars - i just checked out the latest .94 branch, and HBASE-5720-v3.patch applies cleanly for me. Is that the patch you're using for .94?

          Show
          Matt Corgan added a comment - Lars - i just checked out the latest .94 branch, and HBASE-5720 -v3.patch applies cleanly for me. Is that the patch you're using for .94?
          Hide
          Lars Hofhansl added a comment -

          @Matt: Can you make a 0.94 patch? I think your patch is off an older version of trunk rather than 0.94. Thanks.

          Show
          Lars Hofhansl added a comment - @Matt: Can you make a 0.94 patch? I think your patch is off an older version of trunk rather than 0.94. Thanks.
          Hide
          Lars Hofhansl added a comment -

          OK, I'll commit to 0.94 today and create a sub task for further trunk discussion.

          @Matt: +1 on all points (like Stack I do not follow the "project" references in your comment, though). Do you want (and have time) to work out a patch for that?

          Show
          Lars Hofhansl added a comment - OK, I'll commit to 0.94 today and create a sub task for further trunk discussion. @Matt: +1 on all points (like Stack I do not follow the "project" references in your comment, though). Do you want (and have time) to work out a patch for that?
          Hide
          stack added a comment -

          +1 on commit to 0.94. Open a blocker issue for fix on TRUNK.

          @Matt Sounds great. How do we do it? The testable and individually maintained riffs I like. When you say separate directories or projects, what you thinking? It'd be a separate package or even a separate module if we ever get around to modularizing our build. Regards separate project, where you think?

          Show
          stack added a comment - +1 on commit to 0.94. Open a blocker issue for fix on TRUNK. @Matt Sounds great. How do we do it? The testable and individually maintained riffs I like. When you say separate directories or projects, what you thinking? It'd be a separate package or even a separate module if we ever get around to modularizing our build. Regards separate project, where you think?
          Hide
          Matt Corgan added a comment -

          Sounds fine to me. .94 is the important fix, while trunk has some time for more thorough testing, and things will probbaly get moved around even more by other features.

          Eventually it would be nice to isolate the code that deals with the fragile byte[]'s inside HFile blocks into separate src directories or projects. Then have that code implement interfaces similar to DataBlockEncoder.java. This would:

          • make it more testable, like a normal in-memory data structure without having to set up heavyweight testing environments
          • separate the encoding concerns from IO concerns. after the checksum happens, encoders/decoders should not even know what an IOException is
          • strongly discourage people from modifying anything in the codec packages without knowing what they're getting into
          • ensure the main project code only references the interfaces and not any codec internals (see if main project compiles without codecs in classpath)
          • make it easier for contributors to develop and profile the codecs without having to become experts in all aspects of hbase
          • help to simplify the main project. imagine if the gzip or snappy internals were sprinkled throughout the regionserver code. yikes.
          Show
          Matt Corgan added a comment - Sounds fine to me. .94 is the important fix, while trunk has some time for more thorough testing, and things will probbaly get moved around even more by other features. Eventually it would be nice to isolate the code that deals with the fragile byte[]'s inside HFile blocks into separate src directories or projects. Then have that code implement interfaces similar to DataBlockEncoder.java. This would: make it more testable, like a normal in-memory data structure without having to set up heavyweight testing environments separate the encoding concerns from IO concerns. after the checksum happens, encoders/decoders should not even know what an IOException is strongly discourage people from modifying anything in the codec packages without knowing what they're getting into ensure the main project code only references the interfaces and not any codec internals (see if main project compiles without codecs in classpath) make it easier for contributors to develop and profile the codecs without having to become experts in all aspects of hbase help to simplify the main project. imagine if the gzip or snappy internals were sprinkled throughout the regionserver code. yikes.
          Hide
          Lars Hofhansl added a comment -

          That patch looks like it should work. As I mention in an earlier comment, though, this strategy is brittle especially considering future changes. At the same time I cannot think of anything better offhand.

          Let me move the trunk discussion into another jira and commit the 0.94 fix via this one. Everybody OK with that?

          Show
          Lars Hofhansl added a comment - That patch looks like it should work. As I mention in an earlier comment, though, this strategy is brittle especially considering future changes. At the same time I cannot think of anything better offhand. Let me move the trunk discussion into another jira and commit the 0.94 fix via this one. Everybody OK with that?
          Hide
          Hadoop QA added a comment -

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

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

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

          +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/1442//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1442//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1442//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/12521775/5720-trunk-v2.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 new or modified tests. +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/1442//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1442//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1442//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          I ran TestFromClientSide on MacBook with patch v2 and it passed.

          Show
          Ted Yu added a comment - I ran TestFromClientSide on MacBook with patch v2 and it passed.
          Hide
          Hadoop QA added a comment -

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

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

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

          +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 failed these unit tests:
          org.apache.hadoop.hbase.client.TestFromClientSide

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1440//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1440//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1440//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/12521763/5720-trunk.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +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 failed these unit tests: org.apache.hadoop.hbase.client.TestFromClientSide Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1440//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1440//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1440//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          Patch for trunk v2 removes white spaces.

          Show
          Ted Yu added a comment - Patch for trunk v2 removes white spaces.
          Hide
          Ted Yu added a comment -

          I was experimenting along the same direction as Lars outlined.
          Patch is for reference only.

          Show
          Ted Yu added a comment - I was experimenting along the same direction as Lars outlined. Patch is for reference only.
          Hide
          Lars Hofhansl added a comment -

          Yeah, I poked around a few minutes. Seems we need to pass the header at the time when we do the encoding, rather than storing it in the EncodingContext. The changes for that are not entirely trivial.
          An ugly way out might be a setter for the header in the EncodingContext and then set it when known, but that is too nasty and error prone. Maybe Dhruba has an idea...?

          At this point I think I want to commit to and close this for 0.94 and file another jira for trunk. Any objections to that?

          Show
          Lars Hofhansl added a comment - Yeah, I poked around a few minutes. Seems we need to pass the header at the time when we do the encoding, rather than storing it in the EncodingContext. The changes for that are not entirely trivial. An ugly way out might be a setter for the header in the EncodingContext and then set it when known, but that is too nasty and error prone. Maybe Dhruba has an idea...? At this point I think I want to commit to and close this for 0.94 and file another jira for trunk. Any objections to that?
          Hide
          stack added a comment -

          Sorry for attaching non-compiling patch. Looking at this, its hard to get the a default header into the HFileBlockDefaultEncodingContext where it seems it now belongs.

          Show
          stack added a comment - Sorry for attaching non-compiling patch. Looking at this, its hard to get the a default header into the HFileBlockDefaultEncodingContext where it seems it now belongs.
          Hide
          Matt Corgan added a comment -

          Looks like HBASE-5313 shuffles things around by adding the HFileBlockDefaultEncodingContext

          Show
          Matt Corgan added a comment - Looks like HBASE-5313 shuffles things around by adding the HFileBlockDefaultEncodingContext
          Hide
          Lars Hofhansl added a comment -

          v4 does not compile:

          [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java:[74,39] cannot find symbol
          [ERROR] symbol : variable block
          [ERROR] location: class org.apache.hadoop.hbase.io.hfile.HFileDataBlockEncoderImpl

          The "block" indeed is not known to HFileDataBlockEncoderImpl. I'm lacking a bit context to suggest a different fix.

          Show
          Lars Hofhansl added a comment - v4 does not compile: [ERROR] /home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/trunk/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileDataBlockEncoderImpl.java: [74,39] cannot find symbol [ERROR] symbol : variable block [ERROR] location: class org.apache.hadoop.hbase.io.hfile.HFileDataBlockEncoderImpl The "block" indeed is not known to HFileDataBlockEncoderImpl. I'm lacking a bit context to suggest a different fix.
          Hide
          dhruba borthakur added a comment -

          Stack's patch for trunk looks good to me, +1

          Show
          dhruba borthakur added a comment - Stack's patch for trunk looks good to me, +1
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521650/5720v4.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 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The patch appears to cause mvn compile goal to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +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/1427//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1427//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/12521650/5720v4.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 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause mvn compile goal to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +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/1427//testReport/ Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1427//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Trying again.

          Show
          Lars Hofhansl added a comment - Trying again.
          Hide
          stack added a comment -

          Retry

          Show
          stack added a comment - Retry
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521577/5720v4.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 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The patch appears to cause mvn compile goal to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

          +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/1413//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1413//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/12521577/5720v4.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 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause mvn compile goal to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +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/1413//testReport/ Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1413//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Comment crossing again. Ah, I see, yes a problem in trunk too.
          +1 on both patches.

          Show
          Lars Hofhansl added a comment - Comment crossing again. Ah, I see, yes a problem in trunk too. +1 on both patches.
          Hide
          Lars Hofhansl added a comment -

          Is the 1st problem actually a problem in trunk? The code was changed around a lot.

          Show
          Lars Hofhansl added a comment - Is the 1st problem actually a problem in trunk? The code was changed around a lot.
          Hide
          stack added a comment -

          For trunk

          Show
          stack added a comment - 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/12521557/HBASE-5720-v3.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1411//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/12521557/HBASE-5720-v3.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1411//console This message is automatically generated.
          Hide
          Matt Corgan added a comment -

          v3 delays only creates dataBlockEncodingStr if needed

          Show
          Matt Corgan added a comment - v3 delays only creates dataBlockEncodingStr if needed
          Hide
          Lars Hofhansl added a comment -

          Patch doesn't apply to trunk, that's why HadoopQA fails.

          Show
          Lars Hofhansl added a comment - Patch doesn't apply to trunk, that's why HadoopQA fails.
          Hide
          Lars Hofhansl added a comment -

          Minor nit on v2: Don't need to create dataBlockEncodingStr if dataBlockEncodingType is null. I.e. could pull the if statement up.

          I guess it's not reasonable to write a unittest for this.

          +1

          Show
          Lars Hofhansl added a comment - Minor nit on v2: Don't need to create dataBlockEncodingStr if dataBlockEncodingType is null. I.e. could pull the if statement up. I guess it's not reasonable to write a unittest for this. +1
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521547/HBASE-5720-v2.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1408//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/12521547/HBASE-5720-v2.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1408//console This message is automatically generated.
          Hide
          Matt Corgan added a comment -

          v2 patch also reverts the logic in HFileDataBlockEncoderImpl.createFromFileInfo to an earlier version. Version on .94 branch will not allow disk->cache encoding on a .92 because (in short) it doesn't have an encoderId. I'm pretty sure this is something we want to support, but correct me if i'm wrong. Without it you'd have to major compact everything before using encoding (or something along those lines)

          Test suite passing (except unrelated failure), and it works in my benchmarking setup for HBASE-4676

          Show
          Matt Corgan added a comment - v2 patch also reverts the logic in HFileDataBlockEncoderImpl.createFromFileInfo to an earlier version. Version on .94 branch will not allow disk->cache encoding on a .92 because (in short) it doesn't have an encoderId. I'm pretty sure this is something we want to support, but correct me if i'm wrong. Without it you'd have to major compact everything before using encoding (or something along those lines) Test suite passing (except unrelated failure), and it works in my benchmarking setup for HBASE-4676
          Hide
          Matt Corgan added a comment -

          I've confirmed in some isolated test cases, and I'm running the test suite now. I think i have one more fix as well though where .92 files cannot get encoded from disk to memory because the .92 files have no encoderId. Doesn't break anything but skips the desired encoding. I'll add that fix to this patch since they're both related to .92 compatibility.

          Show
          Matt Corgan added a comment - I've confirmed in some isolated test cases, and I'm running the test suite now. I think i have one more fix as well though where .92 files cannot get encoded from disk to memory because the .92 files have no encoderId. Doesn't break anything but skips the desired encoding. I'll add that fix to this patch since they're both related to .92 compatibility.
          Hide
          dhruba borthakur added a comment -

          +1 to this patch.

          I think this bug exists in trunk too. Matt, thanks for finding it. Have you tried this patch in your setup to verify that it fixes the problem for 0.94? If so, i will submit a patch for trunk too.

          Show
          dhruba borthakur added a comment - +1 to this patch. I think this bug exists in trunk too. Matt, thanks for finding it. Have you tried this patch in your setup to verify that it fixes the problem for 0.94? If so, i will submit a patch for trunk too.
          Hide
          Lars Hofhansl added a comment -

          Patch makes sense. Let's get Dhruba to have a look at it.

          Show
          Lars Hofhansl added a comment - Patch makes sense. Let's get Dhruba to have a look at it.
          Hide
          Matt Corgan added a comment -

          that's right, .94, though i assumed the code would still be in trunk

          Show
          Matt Corgan added a comment - that's right, .94, though i assumed the code would still be in trunk
          Hide
          Ted Yu added a comment -

          Looks like the issue only exists in 0.94 branch.
          There is no such code in trunk version of HFileDataBlockEncoderImpl.java:

          *** 194,200 ****
                  DataBlockEncoding algo, boolean includesMemstoreTS) {
                ByteBuffer compressedBuffer = encodeBufferToHFileBlockBuffer(
                    block.getBufferWithoutHeader(), algo, includesMemstoreTS,
          -         HFileBlock.DUMMY_HEADER);
          
          Show
          Ted Yu added a comment - Looks like the issue only exists in 0.94 branch. There is no such code in trunk version of HFileDataBlockEncoderImpl.java: *** 194,200 **** DataBlockEncoding algo, boolean includesMemstoreTS) { ByteBuffer compressedBuffer = encodeBufferToHFileBlockBuffer( block.getBufferWithoutHeader(), algo, includesMemstoreTS, - HFileBlock.DUMMY_HEADER);
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12521445/HBASE-5720-v1.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 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1400//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/12521445/HBASE-5720-v1.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 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1400//console This message is automatically generated.

            People

            • Assignee:
              Matt Corgan
              Reporter:
              Matt Corgan
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development