-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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.
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
HDFS-16748. DFSClient should uniquely identify writing files by namespace id and iNodeId via RBF #4813
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@tomscut @ayushtkn @Hexiaoqiao Hi, masters, can you help me review this patch? |
@goiri Master, can help review this patch? After using RBF, this logic has bugs and needs to be fixed. |
@ZanderXu Thanks involve me here. IIUC, this improvement will help Router to forward renewLease to only one or certain Namenode, right? If that, it makes sense to me. Only one nit comment, title 'by namespace id and iNodeId via RBF' seems not related to updates completely. I would like to hear some other folks' comments. |
@Hexiaoqiao Thanks for your review. This improvement is not used to help Router to forward renewLease to only one or certain Namenode. One DFSClient may be writing some files from different namespace but with the same file id via RBF. The current For example:
It may caused this DFSClient cannot renew the lease of /ns0/file0 |
OK. Got it. |
Just thinking, if the second file which was on top, which overwrote the previous entry gets closed, then there would be no entry & in that case renewLease will not be triggered only for the entry which got overwritten, even without the previous patch? Am I missing something... |
if (this.namespace == null) { | ||
this.renewLeaseKey = "DEFAULT" + "_" + this.fileId; | ||
} else { | ||
this.renewLeaseKey = this.namespace + "_" + this.fileId; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "DEFAULT" needs to be configurable, someone can have a namespace with name DEFAULT as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Sir. I have updated it.
@ayushtkn Nice example. @Hexiaoqiao Maybe I misled you. Incorrect filesBeingWritten can cause a lot of problems, not just renewLease, such as DFSClient lose some writing files when closing all writing files. |
Pretty case. Thanks to correct me. I want to figure out that this case could meet only for RBF deploy, right? |
Yes. |
Great, +1 from my side. Let's wait what Yetus will say. |
💔 -1 overall
This message was automatically generated. |
The failed UT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ayushtkn @Hexiaoqiao Masters, thank you very much for helping me to review this patch. |
…namespace id and iNodeId via RBF (apache#4813). Contributed by ZanderXu. Reviewed-by: He Xiaoqiao <hexiaoqiao@apache.org> Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
Description of PR
DFSClient should uniquely identify the writing files by namespaceId and iNodeId. Because one DFSClient may be writing some files belong to different namespaces at the same time via RBF. If DFSClient only identify the writing files by fileId, it will lose some writing files, because the writing files from different namespaces may has same INodeId.
And the related code as bellows: