HBase
  1. HBase
  2. HBASE-2935

Refactor "Corrupt Data" Tests in TestHLogSplit

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.89.20100621
    • Fix Version/s: 0.90.0
    • Component/s: test
    • Labels:
      None

      Description

      While fixing HBASE-2643, I noticed that a couple of the HLogSplit tests from HBASE-2437 were now failing. 3 tests are trying to detect proper handling of garbage data: testCorruptedFileGetsArchivedIfSkipErrors, testTrailingGarbageCorruptionLogFileSkipErrorsFalseThrows, testCorruptedLogFilesSkipErrorsFalseDoesNotTouchLogs. However, these tests are corrupting data at the HBase level. Data corruption should be tested at the HDFS level, because the filesystem is responsible for data validation. These tests need to inject corrupt data at the HDFS level & then verify that ChecksumExceptions are thrown.

        Issue Links

          Activity

          Hide
          stack added a comment -

          Committed. Thanks for the patch Alex. Good one.

          Show
          stack added a comment - Committed. Thanks for the patch Alex. Good one.
          Hide
          Alex Newman added a comment -

          v2 based on stack's and ryan's input

          Show
          Alex Newman added a comment - v2 based on stack's and ryan's input
          Hide
          HBase Review Board added a comment -

          Message from: "Alex Newman" <newalex@cloudera.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/1115/#review1714
          -----------------------------------------------------------

          src/test/java/org/apache/hadoop/hbase/regionserver/wal/FaultySequenceFileLogReader.java
          <http://review.cloudera.org/r/1115/#comment5641>

          woops pretend this says
          return FailureType.valueOf(conf.get("faultysequencefilelogreader.failuretype", FailureType.NONE.name()));

          • Alex
          Show
          HBase Review Board added a comment - Message from: "Alex Newman" <newalex@cloudera.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1115/#review1714 ----------------------------------------------------------- src/test/java/org/apache/hadoop/hbase/regionserver/wal/FaultySequenceFileLogReader.java < http://review.cloudera.org/r/1115/#comment5641 > woops pretend this says return FailureType.valueOf(conf.get("faultysequencefilelogreader.failuretype", FailureType.NONE.name())); Alex
          Hide
          HBase Review Board added a comment -

          Message from: "Alex Newman" <newalex@cloudera.com>

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          http://review.cloudera.org/r/1115/
          -----------------------------------------------------------

          (Updated 2010-10-29 13:24:54.726844)

          Review request for hbase and Nicolas.

          Summary
          -------

          So I took the approach taken with the instrumented sequence file rider. Not sure if exposing those methods in the WALReader is cool so I am down with other suggestions.

          • Add a FaultySequenceFileLogReader
          • Un-commented the 2935 tests

          This addresses bug hbase-2935.
          http://issues.apache.org/jira/browse/hbase-2935

          Diffs


          src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java c8a10af
          src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java 0b7dc78
          src/test/java/org/apache/hadoop/hbase/regionserver/wal/FaultySequenceFileLogReader.java PRE-CREATION
          src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java 473c359

          Diff: http://review.cloudera.org/r/1115/diff

          Testing
          -------

          Running on our internal hudson as I type this. Also I ran the TestHLogSplit test locally. I'll update the review if hudson is green.

          Thanks,

          Alex

          Show
          HBase Review Board added a comment - Message from: "Alex Newman" <newalex@cloudera.com> ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1115/ ----------------------------------------------------------- (Updated 2010-10-29 13:24:54.726844) Review request for hbase and Nicolas. Summary ------- So I took the approach taken with the instrumented sequence file rider. Not sure if exposing those methods in the WALReader is cool so I am down with other suggestions. Add a FaultySequenceFileLogReader Un-commented the 2935 tests This addresses bug hbase-2935. http://issues.apache.org/jira/browse/hbase-2935 Diffs src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java c8a10af src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceFileLogReader.java 0b7dc78 src/test/java/org/apache/hadoop/hbase/regionserver/wal/FaultySequenceFileLogReader.java PRE-CREATION src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLogSplit.java 473c359 Diff: http://review.cloudera.org/r/1115/diff Testing ------- Running on our internal hudson as I type this. Also I ran the TestHLogSplit test locally. I'll update the review if hudson is green. Thanks, Alex
          Hide
          Alex Newman added a comment -

          If y'all want I can mangle some underlying blocks in a test as well?

          Show
          Alex Newman added a comment - If y'all want I can mangle some underlying blocks in a test as well?
          Show
          Alex Newman added a comment - https://review.cloudera.org/r/1115/
          Show
          Alex Newman added a comment - https://review.cloudera.org/r/1115/
          Hide
          Alex Newman added a comment -

          I should mention I am more than happy to take this one on, I just wanted some clarification first.

          Show
          Alex Newman added a comment - I should mention I am more than happy to take this one on, I just wanted some clarification first.
          Hide
          Alex Newman added a comment -

          At first I believed the same thing. On the other hand we do need to test the checksum exception as it is a code path. How about I do the checksum exception one first and create a new jira for corrupting hdfs. Also, it's my personal opinion that making sure HDFS causes a checksum exception is really a hdfs thing, not a hbase thing.

          Show
          Alex Newman added a comment - At first I believed the same thing. On the other hand we do need to test the checksum exception as it is a code path. How about I do the checksum exception one first and create a new jira for corrupting hdfs. Also, it's my personal opinion that making sure HDFS causes a checksum exception is really a hdfs thing, not a hbase thing.
          Hide
          Nicolas Spiegelberg added a comment -

          3 tests were removed, so this jira is about evaluating whether we should add them back in and to what extent. My suggestion is to run a system-level test, where we corrupt at the HDFS level and ensure that everything percolate up and gets handled properly. Just throwing a ChecksumException is fine to simply test the splitLog() logic but defeats the previous idea that Cosmin had of actually corrupting the underlying file data.

          Show
          Nicolas Spiegelberg added a comment - 3 tests were removed, so this jira is about evaluating whether we should add them back in and to what extent. My suggestion is to run a system-level test, where we corrupt at the HDFS level and ensure that everything percolate up and gets handled properly. Just throwing a ChecksumException is fine to simply test the splitLog() logic but defeats the previous idea that Cosmin had of actually corrupting the underlying file data.
          Hide
          Alex Newman added a comment -

          I am not exactly sure what we are testing with this jira.

          • Are we verifying that hdfs throws the ChecksumException, shouldn't this be an hdfs test?
          • Is it enough to just force cause a checksumException and to make sure the logsplitter handles it correctly?
          Show
          Alex Newman added a comment - I am not exactly sure what we are testing with this jira. Are we verifying that hdfs throws the ChecksumException, shouldn't this be an hdfs test? Is it enough to just force cause a checksumException and to make sure the logsplitter handles it correctly?

            People

            • Assignee:
              Alex Newman
              Reporter:
              Nicolas Spiegelberg
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development