HDFS-16748. DFSClient should uniquely identify writing files by namespace id and iNodeId via RBF #4813
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. |
|
🎊 +1 overall
This message was automatically generated. |
ZanderXu
changed the title
ZanderXu
changed the title
|
@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... |
ayushtkn
reviewed
Sep 1, 2022
Comment on lines
201
to
205
| if (this.namespace == null) { | ||
| this.renewLeaseKey = "DEFAULT" + "_" + this.fileId; | ||
| } else { | ||
| this.renewLeaseKey = this.namespace + "_" + this.fileId; | ||
| } |
There was a problem hiding this comment.
The "DEFAULT" needs to be configurable, someone can have a namespace with name DEFAULT as well
There was a problem hiding this comment.
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 |
|
@ayushtkn @Hexiaoqiao Masters, thank you very much for helping me to review this patch. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…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>
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.
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: