Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-2872

Add sanity checks during edits loading that generation stamps are non-decreasing

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: 1.1.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In 0.23 and later versions, we have a txid per edit, and the loading process verifies that there are no gaps. Lacking this in 1.0, we can use generation stamps as a proxy - the OP_SET_GENERATION_STAMP opcode should never result in a decreased genstamp. If it does, that would indicate that the edits are corrupt, or older edits are being applied to a newer checkpoint, for example.

      1. HDFS-2872.patch
        1 kB
        Colin Patrick McCabe

        Activity

        Hide
        Colin Patrick McCabe added a comment -

        In EditLogFileInputStream, check that our genStamps are always increasing. Add a unit test to verify that we catch this case.

        Show
        Colin Patrick McCabe added a comment - In EditLogFileInputStream, check that our genStamps are always increasing. Add a unit test to verify that we catch this case.
        Hide
        Aaron T. Myers added a comment -

        Hey Colin, it looks like the patch you posted is against trunk, and not branch-1.

        Show
        Aaron T. Myers added a comment - Hey Colin, it looks like the patch you posted is against trunk, and not branch-1.
        Hide
        Colin Patrick McCabe added a comment -

        Yeah, you're right. The bug is against branch-1, so the patch should be too (in this case.)

        I re-did this against branch-1

        Show
        Colin Patrick McCabe added a comment - Yeah, you're right. The bug is against branch-1, so the patch should be too (in this case.) I re-did this against branch-1
        Hide
        Colin Patrick McCabe added a comment -

        the file I meant to append...

        Show
        Colin Patrick McCabe added a comment - the file I meant to append...
        Hide
        Tsz Wo Nicholas Sze added a comment -

        This is a good check. Should it be more strict, i.e. check whether cur == prv + 1?

        Show
        Tsz Wo Nicholas Sze added a comment - This is a good check. Should it be more strict, i.e. check whether cur == prv + 1?
        Hide
        Colin Patrick McCabe added a comment -

        generation stamps don't increase on every EditLog operation.

        However, normally they should either increase by 0 or 1 on each new operation. In theory, we could forbid the generation stamp from ever increasing by more than one, but that would probably make recovery of a corrupted EditLog file more difficult than it is now.

        Show
        Colin Patrick McCabe added a comment - generation stamps don't increase on every EditLog operation. However, normally they should either increase by 0 or 1 on each new operation. In theory, we could forbid the generation stamp from ever increasing by more than one, but that would probably make recovery of a corrupted EditLog file more difficult than it is now.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        > generation stamps don't increase on every EditLog operation.

        Sorry that I am not being clear. I mean for each OP_SET_GENSTAMP, check whether the cur == prv + 1, where cur is the value in the current OP_SET_GENSTAMP and prv is the value in the previous OP_SET_GENSTAMP. For other OPs, no check is needed.

        Show
        Tsz Wo Nicholas Sze added a comment - > generation stamps don't increase on every EditLog operation. Sorry that I am not being clear. I mean for each OP_SET_GENSTAMP, check whether the cur == prv + 1, where cur is the value in the current OP_SET_GENSTAMP and prv is the value in the previous OP_SET_GENSTAMP. For other OPs, no check is needed.
        Hide
        Colin Patrick McCabe added a comment -

        enforce the invariant that genStamps must always increase by 1

        Show
        Colin Patrick McCabe added a comment - enforce the invariant that genStamps must always increase by 1
        Hide
        Eli Collins added a comment -

        Some nits, otherwise looks great.

        • I'd throw IOE instead of InvalidGenStampException to be consisten with the rest of the method
        • You don't need to use StringBuilder since Java optimizes string concats and this code path is rarely run
        • The patch has uses tabs instead of spaces
        Show
        Eli Collins added a comment - Some nits, otherwise looks great. I'd throw IOE instead of InvalidGenStampException to be consisten with the rest of the method You don't need to use StringBuilder since Java optimizes string concats and this code path is rarely run The patch has uses tabs instead of spaces
        Hide
        Colin Patrick McCabe added a comment -
        • fix whitespace issues
        • throw IOE rather than a custom exception
        Show
        Colin Patrick McCabe added a comment - fix whitespace issues throw IOE rather than a custom exception
        Hide
        Colin Patrick McCabe added a comment -
        • remove unrelated file from patch
        Show
        Colin Patrick McCabe added a comment - remove unrelated file from patch
        Hide
        Eli Collins added a comment -

        +1 looks good. I ran a build and reloaded the edits log for sanity.

        Show
        Eli Collins added a comment - +1 looks good. I ran a build and reloaded the edits log for sanity.
        Hide
        Eli Collins added a comment -

        I've committed this. Thanks Colin!

        Show
        Eli Collins added a comment - I've committed this. Thanks Colin!
        Hide
        Matt Foley added a comment -

        Closed upon release of Hadoop-1.1.0.

        Show
        Matt Foley added a comment - Closed upon release of Hadoop-1.1.0.

          People

          • Assignee:
            Colin Patrick McCabe
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development