HDFS-17518: Sync the editslog if a file is closed in the lease monitor #6809
Conversation
...s-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| @@ -626,7 +626,8 @@ private synchronized boolean checkLeases(Collection<Lease> leasesToCheck) { | |||
| } | |||
| } | |||
| // If a lease recovery happened, we need to sync later. | |||
There was a problem hiding this comment.
This is a nice hack. But this will not handle the case, where actual recovery of the file is triggered and lease is reassigned.
Lease re-assignment also will have a edit log. This also should be synced as well.
I would recommend you to change the return type of internalReleaseLease() to ImmutablePair<Boolean, Boolean> to include both completed and needSync values.
needSync will be true in both cases of file closed and lease re-assignment.
There was a problem hiding this comment.
@vinayakumarb Thank you for your review. Let me explain the current logic. The logic I am modifying now is as follows: if the lease is recovered or the lease is reassigned, it will return false, just like the previous logic. Then, in the checkLeases method, if the return is false and needSync is false, needSync will be reset to true. Subsequently, the edits log will be flushed by leaseMonitor. This way, when RPCs such as recoverLease call the internalReleaseLease method, they can remain consistent with the original behavior.
There was a problem hiding this comment.
I would recommend you to change the return type of internalReleaseLease() to ImmutablePair<Boolean, Boolean> to include both completed and needSync values.
needSync will be true in both cases of file closed and lease re-assignment.
+1. If we will plan to improve it, should fix it together.
BTW, what will it happen if not sync in time, LeaseManager.Monitor is one asynchronous logic, it can not be ensure to sync edits in one certain order right?
There was a problem hiding this comment.
On the other hand, if we see the complete logic of internalReleaseLease() we need to call logSync() always.
There are two possibilities overall.
- File gets closed.
- lease recovery gets initiated with reassign of lease.
In both of above cases, there will be edit txn logged. So need to call logSync().
There was a problem hiding this comment.
Thanks, I totally agree to write and sync edit log here where 'write' already do that for both close and reassign lease but miss sync for some corner case. My point is do we need to sync them invoke logSync() in time? And what will it happen if sync by other write operation at following because edit from LeaseManager.Monitor is one asynchronous logic which is not have to in order IMO. Maybe it could be going to be missed if there are no other write operation later then NameNode shutdown?
There was a problem hiding this comment.
@Hexiaoqiao Its perfectly fine if concurrently other write operations also calllogSync(). This transaction also will get included in the logSync() called by other threads. Its true for vice versa as well.
@ThinkerLei I wanted to make the if-else condition simple.
boolean completed=fsn.internalReleaseLease();
if (!needSync)) {
needSync = true;
}
There was a problem hiding this comment.
@vinayakumarb Thank you for your reply. I understand what you mean. But there is another case where calling logSync() is not required as mentioned in HDFS-17519 Do we need to consider this scenario described in HDFS-17519?
There was a problem hiding this comment.
I dont think that special case needs to be handled. If there is no txn, then also calling logSync() wont be a problem.
If there is no edit txn, logSync() will just return without doing anything.
There was a problem hiding this comment.
as mentioned above, please change below logic to call logSync() always.
There was a problem hiding this comment.
@vinayakumarb Thank you for your reply. How about changing the method checkLeases to return true?
In the lease monitor, if a file is closed, method checklease will return true, and then the edits log will not be sync. In my opinion, we should sync the edits log to avoid not synchronizing the state to the standby NameNode for a long time.