-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDFS-16373. Fix MiniDFSCluster restart in case of multiple namenodes. #3756
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
Conversation
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.
How about re-throwing as IOE because ultimately main thread getting interrupted is going to be problematic? throw new IOException(e)
Even without this patch, catching InterruptedException
was no-op but I believe re-throwing it as IOException might help us analyze any hidden bug underneath. Thoughts?
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.
Logically made sense, so I have changed it, In general I don't expect any test to fail due to this, but if any will revert to normal, considering minidfs is used by a lot of downstream projects as well
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/MiniDFSCluster.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/MiniDFSCluster.java
Outdated
Show resolved
Hide resolved
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.
Shall we just do restarts rather than shutdown + restart?
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.
I don't think that will repro the issue, Single restart shall pass only since all other namenodes are up. May be just one shutdown and the other one restarting will repro...
But that won't look good. My original use case was from a RBF patch, where I was restaring the namenodes like this only, so kept the logic same
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.
I don't think that will repro the issue, Single restart shall pass only since all other namenodes are up. May be just one shutdown and the other one restarting will repro...
But that won't look good. My original use case was from a RBF patch, where I was restaring the namenodes like this only, so kept the logic same
Ah I see, yeah makes sense 👍
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.
+1 (non-binding) for the latest patch, pending QA
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
Thanks for fixing it, @ayushtkn. Thanks for your review, @virajjasani and @tomscut. |
…#3756) Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: litao <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
…#3756) Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: litao <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
…apache#3756) Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: litao <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
…#3756) Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: litao <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
…apache#3756) Reviewed-by: Viraj Jasani <[email protected]> Reviewed-by: litao <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
Description of PR
Fixes MiniDFSCluster namenode restart in case of multiple namenodes.
How was this patch tested?
Added UT
For code changes: