Details

Description

Standby NN has got the ledgerlist with list of all files, including the inprogress file (with say inprogress_val1)
Active NN has done finalization and created new inprogress file.
Standby when proceeds further finds that the inprogress file which it had in the list is not present and NN gets shutdown

Uma Maheswara Rao G
added a comment - 31/May/12 20:54 This is what i have committed. Resolved the TestBookKeeperJournalManager conflicts with HDFS-3423 and removed one unused import ArrayList in TestBookKeeperJournalManager.

Hadoop QA
added a comment - 31/May/12 15:15 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12530389/HDFS-3441.3.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 1 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+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 hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2552//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2552//console
This message is automatically generated.

Ivan Kelly
added a comment - 31/May/12 14:10 looks good. Just needs a small tweak in the test.
+ List<String> ledgerNames = new ArrayList<String>(1);
+ ledgerNames.add("editlog_120_121");
ledgerNames is never used now.

Hadoop QA
added a comment - 31/May/12 11:16 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12530361/HDFS-3441.2.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 1 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+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 hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2550//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2550//console
This message is automatically generated.

the code of the patch looks good. I think the test should be a little lower level and more explicit though. Its not clear that the new code path is actually getting exercised. Make getLedgerList() package private, and test that when you getChildren() returns a list of ledgers, and some do not exist when the read occurs, that you still get the others back fine. For this you would have to use spy() instead of mock(), and just stub out the getData() method.

Ivan Kelly
added a comment - 30/May/12 14:39 the code of the patch looks good. I think the test should be a little lower level and more explicit though. Its not clear that the new code path is actually getting exercised. Make getLedgerList() package private, and test that when you getChildren() returns a list of ledgers, and some do not exist when the read occurs, that you still get the others back fine. For this you would have to use spy() instead of mock(), and just stub out the getData() method.

Hadoop QA
added a comment - 30/May/12 13:50 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12530189/HDFS-3441.1.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 1 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+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 hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2544//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2544//console
This message is automatically generated.

Rakesh R
added a comment - 18/May/12 16:44 I'm attaching initial patch for reference, this is what I have in my mind. Please review the fix. I'll add few tests.
This has to be applied on top of HDFS-3058 .
When reviewing, I have seen few more race condition between
1) finalizeLogSegment() and getInputStreams(),
2) finalizeLogSegment() and getNumberOfTransactions()
That is the reason I have ignored the NoNodeException in the patch. I feel, ignoring and continuing would not cause any inconsistencies.

Uma Maheswara Rao G
added a comment - 18/May/12 13:37 Similar issue can exist in FileJournalManager, but we are just ignoring any issues on delete.
if (!file.delete())
{
// It's OK if we fail to delete something -- we'll catch it
// next time we swing through this directory.
LOG.warn("Could not delete " + file);
}
Yes, Agree with Rakesh, We can just ignore it.

Yeah, I have seen a race condition between the purgeLogsOlderThan() by Standby and the finalizeLogSegment() by Active.

Cause: Following are the sequence of operations happening:
1) When standby comes to purge, it is reading all the list of ledger logs including the inprogress_72 files.
2) Meantime Active NN is finalizing the logSegment inprogress_72 and creating new inprogress_74.
3) Now the Standby is reading the data of inprogress_72 to decide whether its inprogress or not and is throwing NoNodeException.

I feel, the filtering of inprogress file could be done based on the file name itself, instead of reading the content and filtering based on the data like as follows:

Rakesh R
added a comment - 18/May/12 07:51 Yeah, I have seen a race condition between the purgeLogsOlderThan() by Standby and the finalizeLogSegment() by Active.
Cause: Following are the sequence of operations happening:
1) When standby comes to purge, it is reading all the list of ledger logs including the inprogress_72 files.
2) Meantime Active NN is finalizing the logSegment inprogress_72 and creating new inprogress_74.
3) Now the Standby is reading the data of inprogress_72 to decide whether its inprogress or not and is throwing NoNodeException.
I feel, the filtering of inprogress file could be done based on the file name itself, instead of reading the content and filtering based on the data like as follows:
BookKeeperJournalManager.java
List<String> ledgerNames = zkc.getChildren(ledgerPath, false);
for (String ledgerName : ledgerNames) {
if( !inProgressOk && ledgerName.contains("inprogress") ){
continue;
}
ledgers.add(EditLogLedgerMetadata.read(zkc, ledgerPath + "/" + ledgerName));
}
} catch (Exception e) {
throw new IOException("Exception reading ledger list from zk", e);
}