-
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-16338. Correct fsimage error configuration message #3684
Conversation
💔 -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.
Thanks @GuoPhilipse for the improvement, +1
🎊 +1 overall
This message was automatically generated. |
@cxorm Thanks for your review :) |
@@ -275,6 +276,31 @@ public void testSaveAndLoadStripedINodeFile() throws IOException{ | |||
} | |||
} | |||
|
|||
@Test | |||
public void testImportCheckpoint() throws IOException{ |
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.
A little advice. You can consider using try with resources for MiniDFSCluster cluster = new MiniDFSCluster.Builder(config).build()
.
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 @tomscut for your advice , it seems simpler code.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -587,7 +587,7 @@ void doImportCheckpoint(FSNamesystem target) throws IOException { | |||
|
|||
if (checkpointEditsDirs == null || checkpointEditsDirs.isEmpty()) { | |||
throw new IOException("Cannot import image from a checkpoint. " | |||
+ "\"dfs.namenode.checkpoint.dir\" is not set." ); | |||
+ "\"dfs.namenode.checkpoint.edits.dir\" is not set." ); |
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.
Looks like there are some checktyles that need fixing. Maybe you can configure checkstyle.xml
in your IDE. See ./hadoop-github/hadoop-build-tools/src/main/resources/checkstyle/checkstyle.xml
. The other changes look good to me.
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.
great! Thanks @tomscut for your advice,
🎊 +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.
...fs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSImage.java
Outdated
Show resolved
Hide resolved
fail("Expect to throw IOException."); | ||
} catch (IOException e) { | ||
GenericTestUtils.assertExceptionContains( | ||
"Cannot import image from a checkpoint. " | ||
+ "\"dfs.namenode.checkpoint.edits.dir\" is not set.", e); |
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.
Use LambdaTestUtils instead of try-catch-assert
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 @ayushtkn for your advice, just updated.
💔 -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.
Changes LGTM.
Will commit if no complains from Jenkins...
💔 -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.
Checkstyle needs to be corrected:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3684/10/artifact/out/results-checkstyle-hadoop-hdfs-project_hadoop-hdfs.txt
Thanks @ayushtkn for your check, have just updated. |
🎊 +1 overall
This message was automatically generated. |
Merged, Thanx Everyone!!! |
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.
Belated +1 (non-binding). Thanks @GuoPhilipse. Left one minor nit if you would like to take it up later. Thanks
@@ -582,12 +582,12 @@ void doImportCheckpoint(FSNamesystem target) throws IOException { | |||
|
|||
if (checkpointDirs == null || checkpointDirs.isEmpty()) { | |||
throw new IOException("Cannot import image from a checkpoint. " | |||
+ "\"dfs.namenode.checkpoint.dir\" is not set." ); | |||
+ "\"dfs.namenode.checkpoint.dir\" is not set."); | |||
} | |||
|
|||
if (checkpointEditsDirs == null || checkpointEditsDirs.isEmpty()) { |
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.
Btw checkpointDirs
and checkpointEditsDirs
will not be null as they are assigned values returned by FSImage.getCheckpointDirs
and FSImage.getCheckpointEditsDirs
respectively.
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.
hmm, my IntelliJ was giving that warning as well, but that was something not touched here....
I thought Todd added it just for future safety, Didn't research why there was a null check considering it harmless.
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.
Yeah it seems fine, just an additional null check, not a big deal. Also good to keep it for future safety as you predicted correctly, just in case if the methods themselves start returning null (less likely but still doable).
…. Contributed by guophilipse. Reviewed-by: cxorm <lianp964@gmail.com> Reviewed-by: tomscut <litao@bigo.sg> Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
dfs.namenode.checkpoint.edits.dir
may be different fromdfs.namenode.checkpoint.dir
, ifcheckpointEditsDirs
is null or empty, error message should warn the edit dir configuration, we can fix it.