The testFuzzSequences unit test fails without this patch when MAX_INVALID_SEQ = 50000

some minor fixes to v1

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Fix compilation error

Hi Colin, thanks for working on this. Seems like the idea is to, for length-prefixed edit logs, to check the checksum before reading the fields. Sensible and good change, overall looks good. Mostly nit comments:

• A comment mentions a 1-byte txid, I think you meant opid. Having the expected format in the comment is super helpful too, thanks. Mind replicating it for the other two readers too?
• Why move the temp array into a member? It'll probably be stack allocated and thus no GC pressure, 4KB should fit too.
• Great opportunity to fix the LayoutVersion.EDITS_CHESKUM typo
• LengthPrefixedReader#scanOp, shouldn't this be basically readOp but without the second pass to readFields? Not sure why it doesn't validate the max edit size or the checksum. The TODO is relevant.
• Can we get a unit test for this situation?
• Is some code sharing is possible among the three readOp's for getting the FSEditLogOp, this bit?

byte opCodeByte;
      try {
        opCodeByte = in.readByte();
      } catch (EOFException eof) {
        // EOF at an opcode boundary is expected.
        return null;
      }
      FSEditLogOpCodes opCode = FSEditLogOpCodes.fromByte(opCodeByte);
      if (opCode == OP_INVALID) {
        verifyTerminator();
        return null;
      }
      FSEditLogOp op = cache.get(opCode);
      if (op == null) {
        throw new IOException("Read invalid opcode " + opCode);
      }

-1 overall

This message was automatically generated.

Thanks for the review.

A comment mentions a 1-byte txid, I think you meant opid. Having the expected format in the comment is super helpful too, thanks. Mind replicating it for the other two readers too?

sure

Why move the temp array into a member? It'll probably be stack allocated and thus no GC pressure, 4KB should fit too.

Hmm. I'm not sure if it will be stack allocated or not. The compiler would have to do some pretty fancy escape analysis since we're passing the array to many functions, any of which might hold a reference. It seems easier just to keep the buffer around, since we're using it a lot.

Great opportunity to fix the LayoutVersion.EDITS_CHESKUM typo

ok

LengthPrefixedReader#scanOp, shouldn't this be basically readOp but without the second pass to readFields? Not sure why it doesn't validate the max edit size or the checksum. The TODO is relevant.

Yeah, you're right... we should be validating checksums here. Fixed.

It looks like this was actually a regression in edit log validation introduced by HDFS-6038. Previously, the JN did validate checksums when attempting to skip to a specific op, and after that change, it didn't.

Can we get a unit test for this situation?

Yeah, let me add a unit test that scanOp validates checksums. It should be possible to do as a true unit test too.

Is some code sharing is possible among the three readOp's for getting the FSEditLogOp, this bit?

I'm on the fence about this since I think it might make it harder to read since the control flow would be passing from base->derived->base again.

Hmm. I'm not sure if it will be stack allocated or not...

I did a little background research about Java escape analysis and it's pretty fancy, it can even stack allocate across function calls once things are inlined. Our temp array is used for a read loop within a function, so the scope looks a lot like the test program on this page[1] which gets stack allocated. If we trust in the JVM for this, I'd prefer to see local variables rather than members.

[1] http://www.burnison.ca/articles/escape-analysis-stack-allocated-objects

Added checksumming for scanOp. I added a unit test that scanOp now verifies checksums, and verified that it failed on trunk but passed with the patch. It's a true unit test which doesn't start a miniDFSCluster. Fixed the typo in EDITS_CHEKSUM.

I agree that there is a very good chance that a large array would be allocated on the stack. But there's also a chance that it wouldn't. Since the difference in verboseness is negligable (its a single extra line) it seems like we should just allocate it inside the Reader.

Reeeeeally sure that this would get stack allocated but I'll let it slide. Noticed the other two readers don't have a format comment, but that's not a big deal either I guess.

Do we also already have tests for invalid op lengths (e.g. greater than max op size)? I see testFuzzSequences but that's not explicit.

Otherwise LGTM +1 pending

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Do we also already have tests for invalid op lengths (e.g. greater than max op size)? I see testFuzzSequences but that's not explicit.

TestNameNodeRecovery#testNonDefaultMaxOpSize tests maximum op sizes.

The latest patch fixes the test failure in TestJournal. The issue was that we need to ensure that scanOp works when the edit log version is newer than the latest version.

-1 overall

This message was automatically generated.

Thanks Colin P. McCabe for working on this issue..Great work here!!!

Some Minor nits :

1)

in.skip(MIN_OP_LENGTH - 1 - CHECKSUM_LENGTH); // skip length and txid

I think it will be easy to understand code like in.skip (4+8) compared to current statement. what do you say..?

2)

-     * @param logVersion The version of the data coming from the stream.

Seems to be by mistake you might have removed this..?

3)

     FSIMAGE_CHECKSUM(-26, "Support checksum for fsimage"),
     REMOVE_REL13_DISK_LAYOUT_SUPPORT(-27, "Remove support for 0.13 disk layout"),
-    EDITS_CHESKUM(-28, "Support checksum for editlog"),
+    EDITS_CHEKSUM(-28, "Support checksum for editlog"),

I feel, it can be EDITS_CHECKSUM where it will be similar to FSIMAGE_CHECKSUM and as Andrew Wang mentioned,it is good chance to correct this typo..

4)

+     * 1-byte opid

how about change to opcode instead of opid which we did not use..?

5)

The patch appears to introduce 1 new Findbugs (version 3.0.0) warnings.

I think it will be easy to understand code like in.skip (4+8) compared to current statement. what do you say..?

ok

Seems to be by mistake you might have removed [the logVersion JavaDoc]

Fixed, thanks

I feel, it can be EDITS_CHECKSUM where it will be similar to FSIMAGE_CHECKSUM and as Andrew Wang mentioned,it is good chance to correct this typo..

Yes, it seems that I didn't actually correct the typo! Fixed.

how about change [the comment about opid to opcode]

Yes, we refer to "opcode" in other places, so why not here. Changed.

Currently jenkins report will not show this findbug warning,once after committed this patch,we can see..can you take care..? had seen like HDFS-8969...

It looks like the findbugs warning was for Hadoop common, not Hadoop-HDFS, which was a bit confusing. I was able to find out that the warning was:

org.apache.hadoop.hdfs.server.namenode.FSEditLogOp$LengthPrefixedReader.decodeOp() ignores result of java.io.DataInputStream.skip(long)

I have replaced the call to skip with a call to IOUtils#skipFully.

-1 overall

This message was automatically generated.

missed a skip()

LGTM +1

-1 overall

This message was automatically generated.

TestDNFencingWithReplication and bkjm test failures are unrelated to this change and did not reproduce for me locally. Checkstyle issues are more whines about file length and complaining about package-private variables. Committed to 2.8. Thanks, all!

FAILURE: Integrated in Hadoop-trunk-Commit #8375 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8375/)
HDFS-8965. Harden edit log reading code against out of memory errors (cmccabe) (cmccabe: rev 24f6a7c9563757234f53ca23e12f9c9208b53082)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LayoutVersion.java
• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java

Colin P. McCabeThanks for addressing all the comments neatly...+1 ( non-binding) on latest patch..

FAILURE: Integrated in Hadoop-Yarn-trunk #1059 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1059/)
HDFS-8965. Harden edit log reading code against out of memory errors (cmccabe) (cmccabe: rev 24f6a7c9563757234f53ca23e12f9c9208b53082)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LayoutVersion.java
• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #325 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/325/)
HDFS-8965. Harden edit log reading code against out of memory errors (cmccabe) (cmccabe: rev 24f6a7c9563757234f53ca23e12f9c9208b53082)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LayoutVersion.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java

SUCCESS: Integrated in Hadoop-Yarn-trunk-Java8 #332 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/332/)
HDFS-8965. Harden edit log reading code against out of memory errors (cmccabe) (cmccabe: rev 24f6a7c9563757234f53ca23e12f9c9208b53082)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LayoutVersion.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2274 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2274/)
HDFS-8965. Harden edit log reading code against out of memory errors (cmccabe) (cmccabe: rev 24f6a7c9563757234f53ca23e12f9c9208b53082)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LayoutVersion.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #316 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/316/)
HDFS-8965. Harden edit log reading code against out of memory errors (cmccabe) (cmccabe: rev 24f6a7c9563757234f53ca23e12f9c9208b53082)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LayoutVersion.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java

FAILURE: Integrated in Hadoop-Hdfs-trunk #2255 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2255/)
HDFS-8965. Harden edit log reading code against out of memory errors (cmccabe) (cmccabe: rev 24f6a7c9563757234f53ca23e12f9c9208b53082)

• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/LayoutVersion.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLogFileInputStream.java

Hi Colin P. McCabe,

Thanks for your earlier work on this issue. I have a question, sounds like that the edit log is somehow corrupted, and we tried to avoid out-of-memory issue when loading the edit log. Two questions:
1. What do we do with the corrupted edit log entry? skip it?
2. Do we have idea about whether it is a bug that corrupt edit log, or is it a disk/network problem that corrupt the edit log?

Thanks.

1. What do we do with the corrupted edit log entry? skip it?

We only would attempt to skip corrupted edit log entries if recovery mode were turned on. Otherwise, an exception would be thrown, which would ordinarily lead to us trying to read a different copy of the edit log.

2. Do we have idea about whether it is a bug that corrupt edit log, or is it a disk/network problem that corrupt the edit log?

In general, I don't see how we could know the answer to that question without more information.