-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

How does it actually work? Can it replay the same edit log segment multiple times? It might have to replay the same in-progress segment multiple times or after it is finalized.
I thought it would blow up with MetaRecoveryContext being null.

Maybe I got something wrong, but isn't the MetaRecoveryContext already null at the beginning?

In doTailEdits() from EditLogTailer, we have
streams = editLog.selectInputStreams(lastTxnId + 1, 0, null, inProgressTail);

Here, MetaRecoverContext is null. And in image.LoadEdits(), we have
public long loadEdits(Iterable<EditLogInputStream> editStreams,
FSNamesystem target) throws IOException

MetaRecoveryContext is also null.

Also, I don't know why toAtLeastTxId is always set to 0 in the implementation, making the checkForGaps() method no use. If we set it to some other value, like lastTxnId + 1, the multiple replay should be rejected.

Add a fake sendedits to make committed txn id more up-to-date. This means the standby namenode will only tail the in-progress edits which are committed. Thank you Todd Lipcon for the idea during an offline discussion.

Add a unit test which I forgot to add in the 003 patch.

-1 overall

This message was automatically generated.

Fix bugs and checkstyle problems. Add a boolean flag in QuorumOutputStream so that fake send will be triggered only when in-progress tailing is enabled.

-1 overall

This message was automatically generated.

Also add a boolean flag in Journal for the same purpose.

+1 overall

This message was automatically generated.

Add a new parameter isTail to selectInputStream() on the NameNode side and a field in RemoteEditLogManifest. When we do in-progress tailing, we'll use committedTxnId rather than highestWrittenTxnId. This won't affect other parts which also need to select in-progress edits.

Hi Jiayi, thanks for working on this. I took a look at the patch and have mostly style comments. It looks pretty good overall, appreciate the new tests.

• config key should mention "edit log" somewhere. We can reuse an existing prefix and call it something like "dfs.ha.tail-edits.in-progress"
• Would be good to explain a little more in hdfs-default.xml why a user might want to turn this on.
• EditLogTailer, regarding the comment: edits themselves are not in-progress, an edit log segment is. Consider renaming the boolean also.
• "isTail" is also not very descriptive, what it's really doing is bounding the tailing to the committed txID length right? Better to name it accordingly, rather than tie it to a single usecase like standby tailing.

Journal.java

• inProgress boolean is unused. Conceptually the JNs shouldn't have to be aware of the standby doing in-progress tailing anyway.
• getCommittedTxnIdForTests isn't used only in tests, rename it?
• Can you add a comment on the numTxns == 0 early return, explaining why we do this?

• JournalSet, why is the correct committed txnid to set here 0 rather than the max txId from the set of logs?

QuorumOutputStream

• Let's pull the boolean out of the conf and pass it into the constructor rather than pass in the entire Configuration, this helps limit the scope. The boolean should also be named something more descriptive, like "updateCommittedTxnId".
• The new conditional dummy flush also needs a comment for sure. How stale would the committed txnId be if we didn't write this 0-len segment? Any ideas on how to further optimize this (e.g. make it more async)?

• QuorumJournalManager, the ternary can be written instead with Math.min yea? I'd prefer to see a little logic rework that avoids the need for the else also.

RemoteEditLogManifest

• Do we need the single-arg form of the constructor? We only use it once in a test, maybe just always require the two-arg form.
• Initialize committedTxnId to some invalid txn id, and validate it in checkState? Should also be within bounds.
• toString output should included the committedTxnId

• TestBKSR, TestFJM, you don't need to modify the call since we kept the old overload.
• TestNNSRManager, we should pass through the parameter in the mock, like the other args

TestStandbyInProgressTail

• Typo: edig
• Typo: shoudl
• Do we need the Thread.sleep for tailing? Can we trigger edit log tailing manually instead? Sleeps are unreliable and increase test runtime.
• Looks like some of the startup/shutdown logic is shared, can we use @Before and @After annotated methods to share code? As an FYI we also normally do a null check on cluster before calling cluster.shutdown, as an extra guard.
• New assert methods can be made static
• Any thoughts on hooking this up to the randomized fault-injection tests originally developed for QJM? I think we'd all have more confidence in this feature if we ran the fuzzer for a while and it came back clean.

Thanks Andrew for the review. I have attached a new patch to solve the problems.

• Config key name fixed.
• Comment added in hdfs-default.xml. Users want to turn it on if they want more up-to-date data on SBNN.

  EditLogTailer, regarding the comment: edits themselves are not in-progress, an edit log segment is. Consider renaming the boolean also.

• Fixed. Rename to inProgressOk which is just the parameter name in selectInputStream()

  "isTail" is also not very descriptive, what it's really doing is bounding the tailing to the committed txID length right?

• Change it to isBoundedByDurableTxId. It looks a bit long. Any suggestions?

Journal.java

• Unused boolean has been removed
• Rename to getCommittedTxnId
• Comment added. numTxn = 0 implies it's a dummy edits send, so we can return early.

  JournalSet, why is the correct committed txnid to set here 0 rather than the max txId from the set of logs?

• Good point. I lazily set it to 0 since it would not be used. Change it to the max txId from the logs.

QuorumOutputStream

• Fixed. Boolean has been changed to updateCommittedTxnId
• Comments added. Committed txnId could be one-batch stale if we don't have this dummy flush. A thread could be added to do this dummy flush, but setting a proper time period needs to be considered. For now, I prefer to leave it sync.

  QuorumJournalManager, the ternary can be written instead with Math.min yea? I'd prefer to see a little logic rework that avoids the need for the else also.

• Fixed.

RemoteEditLogManifest

• Remove single-arg form of constructor.
• Add a validation in checkState. committedTxnId should be larger than the start TxId of the logs.
• toString() modified.

  TestBKSR, TestFJM, you don't need to modify the call since we kept the old overload.

• Fixed. In TestFJM, parameter is JournalManager, changed it to FileJournalManager.

  TestNNSRManager, we should pass through the parameter in the mock, like the other args

• Fixed.

TestStandbyInProgressTail

• Typos fixed
• Thread.sleep removed. Use manual doTailEdits() instead.
• @Before and @After methods added. Null check is included in tearDown()
• Assert methods are static now.
• Randomized tests could be tricky. I haven't come up with a good idea yet. A new jira might be filed for it in the future.

Thanks for revving Jiayi. Overall looks good, mostly just nits now. I think the next rev should do it.

• in DFSConfigKeys, let's group the new config key with the other "dfs.ha.tail-edits" keys
• Regarding isBoundedByDurableTxId, maybe shorten to onlyDurableTxns? Should update javadoc for JournalSet#selectInputStreams also to talk about the boolean. Would be good to explain what a "durable TxId" means also, and the tightness of the bound (it can be conservative).
• Nit: in QuorumJournalManager, we already assigned remoteLog.getEditTxId() to endTxId. So we can re-use endTxId when doing Math.min.
• QuorumOutputStream, recommend we add the new boolean at the end of the list of parameters, we generally try to put flags/options at the end
• RemoteEditLogManifest, the manifest might not have any logs, then the logs.get(0) check will fail.

I would like to think about those randomized tests more, but yea that can happen in another JIRA.

Fix some problems according to Andrew's recent comment.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Fix some style problems. Remove committedTxId check from RemoteEditLogManifest. Since we don't have dummy send in normal cases, committedTxId will not be up-to-date (especially for a newly-setup cluster).

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Fix style problems. Failed test is unrelated.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Failed test is unrelated. It is filed in another jira. HDFS-10696
Andrew Wang Can you take a look at the patch and see if we could commit it now?

Just some nits, I think this really is the last one:

• Some extra whitespace changes in RemoteEditLogManifest. Looks like there are a few other places we could undo the whitespace changes too.
• Rather than adding this new logString method, shall we update the test strings instead? This method doesn't seem useful except to avoid updating the tests.

Undo some whitespace changes and update test strings instead of adding a new method.

LGTM, thanks Jiayi! +1 pending Jenkins.

-1 overall

This message was automatically generated.

Committed to trunk and branch-3.0.0-alpha1. Thanks for the contribution Jiayi!

I think this would be pretty easy to backport to branch-2 if there's demand, just a few small conflicts.

SUCCESS: Integrated in Hadoop-trunk-Commit #10166 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10166/)
HDFS-10519. Add a configuration option to enable in-progress edit log (wang: rev 098ec2b11ff3f677eb823f75b147a1ac8dbf959e)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorageRetentionManager.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LogsPurgeable.java
• hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperJournalManager.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalSet.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/Journal.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestFailureToReadEdits.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocolPB/PBHelper.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/RemoteEditLogManifest.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupJournalManager.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/SecondaryNameNode.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/client/QuorumJournalManager.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFileJournalManager.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/protocolPB/TestPBHelper.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestStandbyInProgressTail.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestEditLog.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestNNStorageRetentionManager.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournal.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/EditLogTailer.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/client/QuorumOutputStream.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/proto/HdfsServer.proto