Skip to content

HDFS-16507. [SBN read] Avoid purging edit log which is in progress #4082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sunchao merged 2 commits into from
Mar 31, 2022
Merged

HDFS-16507. [SBN read] Avoid purging edit log which is in progress #4082

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e62e45b
HDFS-16507. Avoid purging edit log which is in progress
tomscut Mar 18, 2022
e24a429
use Preconditions.checkArgument instead of assert
tomscut Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions ...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1509,13 +1509,18 @@ synchronized void abortCurrentLogSegment() {
* effect.
*/
@Override
public synchronized void purgeLogsOlderThan(final long minTxIdToKeep) {
public synchronized void purgeLogsOlderThan(long minTxIdToKeep) {
Copy link
Contributor

@virajjasani virajjasani Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once done, we can also revert this.

Copy link
Contributor Author

@tomscut tomscut Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Thank you @virajjasani for your review.

// Should not purge logs unless they are open for write.
// This prevents the SBN from purging logs on shared storage, for example.
if (!isOpenForWrite()) {
return;
}


// Reset purgeLogsFrom to avoid purging edit log which is in progress.
if (isSegmentOpen()) {
minTxIdToKeep = minTxIdToKeep > curSegmentTxId ? curSegmentTxId : minTxIdToKeep;
Copy link
Contributor

@xkrogen xkrogen Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just below this we have:

    assert curSegmentTxId == HdfsServerConstants.INVALID_TXID || // on format this is no-op
      minTxIdToKeep <= curSegmentTxId :
      "cannot purge logs older than txid " + minTxIdToKeep +
      " when current segment starts at " + curSegmentTxId;

We assert that minTxIdToKeep <= curSegmentTxId. So in the situation you described where minTxIdToKeep > curSegmentTxId, shouldn't the assert fail?

Copy link
Contributor Author

@tomscut tomscut Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just below this we have:

    assert curSegmentTxId == HdfsServerConstants.INVALID_TXID || // on format this is no-op
      minTxIdToKeep <= curSegmentTxId :
      "cannot purge logs older than txid " + minTxIdToKeep +
      " when current segment starts at " + curSegmentTxId;

We assert that minTxIdToKeep <= curSegmentTxId. So in the situation you described where minTxIdToKeep > curSegmentTxId, shouldn't the assert fail?

Thank you @xkrogen very much for your reply.

We did add assertion. But in the production, assertions are usually disabled and do not actually take effect. I think we should add strict logic to avoid this problem. What do you think of this?

Copy link
Contributor Author

@tomscut tomscut Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sunchao @tasanuma , could you please take a look at this discussion. Thanks.

Copy link
Contributor Author

@tomscut tomscut Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jojochuang @Hexiaoqiao @ayushtkn , please also take a look. Thank you very much.

This problem begin from inprogress edits tail. And this issue HDFS-14317 does a good job of avoiding this problem.

However, if SNN's rolledit operation is disabled accidentally by configuration, and ANN's automatic roll period is very long, then edit log which is in progress may also be purged.

Although we add assertions, assertion is generally disabled in a production(we don't normally add -ea to JVM parameters). This problem and the logs also prove that we are not strictly ensure(inTxIdToKeep <= curSegmentTxId). So it is dangerous for NameNode. It may crash the active NameNode, because of "No log file to Finalize at Transaction ID xxx".

2022-03-15 17:28:52,867 FATAL namenode.FSEditLog (JournalSet.java:mapJournalsAndReportErrors(393)) - Error: finalize log segment 24207987, 27990692 failed for required journal (JournalAndStream(mgr=QJM
 to [xxx:8485, xxx:8485, xxx:8485, xxx:8485, xxx:8485], stream=null))
org.apache.hadoop.hdfs.qjournal.client.QuorumException: Got too many exceptions to achieve quorum size 3/5. 5 exceptions thrown:
10.152.124.157:8485: No log file to finalize at transaction ID 24207987 ; journal id: ambari-test
        at org.apache.hadoop.hdfs.qjournal.server.Journal.finalizeLogSegment(Journal.java:656)
        at org.apache.hadoop.hdfs.qjournal.server.JournalNodeRpcServer.finalizeLogSegment(JournalNodeRpcServer.java:210)
        at org.apache.hadoop.hdfs.qjournal.protocolPB.QJournalProtocolServerSideTranslatorPB.finalizeLogSegment(QJournalProtocolServerSideTranslatorPB.java:205)
        at org.apache.hadoop.hdfs.qjournal.protocol.QJournalProtocolProtos$QJournalProtocolService$2.callBlockingMethod(QJournalProtocolProtos.java:28890)
        at org.apache.hadoop.ipc.ProtobufRpcEngine$Server$ProtoBufRpcInvoker.call(ProtobufRpcEngine.java:550)
        at org.apache.hadoop.ipc.RPC$Server.call(RPC.java:1094)
        at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1066)
        at org.apache.hadoop.ipc.Server$RpcCall.run(Server.java:1000)
        at java.security.AccessController.doPrivileged(Native Method)
        at javax.security.auth.Subject.doAs(Subject.java:422)
        at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1730)
        at org.apache.hadoop.ipc.Server$Handler.run(Server.java:2989)

We should reset minTxIdToKeep to ensure that the in progress edit log is not purged very strict. And wait for ANN to automatically roll to finalize the edit log. Then, after checkpoint, ANN automatically purged the finalized editlog(See the stack mentioned above).

Copy link
Contributor Author

@tomscut tomscut Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @virajjasani , please also take a look. Thanks a lot.

Copy link
Contributor

@virajjasani virajjasani Mar 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomscut I agree that assert alone is not a good idea because not all prod systems have it enabled. I believe we should replace assert here with Preconditions.checkArgument(), then we don't need this condition here.

}

assert curSegmentTxId == HdfsServerConstants.INVALID_TXID || // on format this is no-op
minTxIdToKeep <= curSegmentTxId :
"cannot purge logs older than txid " + minTxIdToKeep +
Expand Down