HDFS-16507. [SBN read] Avoid purging edit log which is in progress #4082
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
xkrogen
reviewed
Mar 18, 2022
|
|
||
| // Reset purgeLogsFrom to avoid purging edit log which is in progress. | ||
| if (isSegmentOpen()) { | ||
| minTxIdToKeep = minTxIdToKeep > curSegmentTxId ? curSegmentTxId : minTxIdToKeep; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 whereminTxIdToKeep > 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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Hi @virajjasani , please also take a look. Thanks a lot.
There was a problem hiding this comment.
@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.
tomscut
changed the title
virajjasani
reviewed
Mar 28, 2022
|
|
||
| // Reset purgeLogsFrom to avoid purging edit log which is in progress. | ||
| if (isSegmentOpen()) { | ||
| minTxIdToKeep = minTxIdToKeep > curSegmentTxId ? curSegmentTxId : minTxIdToKeep; |
There was a problem hiding this comment.
@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.
| @@ -1509,13 +1509,18 @@ synchronized void abortCurrentLogSegment() { | |||
| * effect. | |||
| */ | |||
| @Override | |||
| public synchronized void purgeLogsOlderThan(final long minTxIdToKeep) { | |||
| public synchronized void purgeLogsOlderThan(long minTxIdToKeep) { | |||
There was a problem hiding this comment.
Once done, we can also revert this.
There was a problem hiding this comment.
Good suggestion. Thank you @virajjasani for your review.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
In order not to affect the basic logic, I introduced the suggestion @virajjasani mentioned. Just change the @tasanuma @Hexiaoqiao @ayushtkn Please also take a look at this. Thanks a lot. |
virajjasani
approved these changes
Mar 30, 2022
sunchao
approved these changes
Mar 31, 2022
|
Merged, thanks! |
|
Thanks @sunchao for the review. |
sunchao
pushed a commit
that referenced
this pull request
Mar 31, 2022
sunchao
pushed a commit
that referenced
this pull request
Mar 31, 2022
steveloughran
pushed a commit
to steveloughran/hadoop
that referenced
this pull request
Apr 5, 2022
steveloughran
pushed a commit
to steveloughran/hadoop
that referenced
this pull request
Apr 12, 2022
asfgit
pushed a commit
that referenced
this pull request
Apr 13, 2022
asfgit
pushed a commit
that referenced
this pull request
Apr 14, 2022
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
LiuGuH
pushed a commit
to LiuGuH/hadoop
that referenced
this pull request
Mar 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA: HDFS-16507.
Avoid purging edit log which is in progress. This is described in detail in JIRA.
The stack: