-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-17072. DFSAdmin: add a triggerDirectoryScanner command. #5825
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
|
💔 -1 overall
This message was automatically generated. |
886af14 to
5577c8d
Compare
|
@ayushtkn @Hexiaoqiao @tomscut @zhangshuyan0 Sir, could you please help me review this PR when you have free time? Thanks a lot. |
|
💔 -1 overall
This message was automatically generated. |
5577c8d to
77dad80
Compare
|
💔 -1 overall
This message was automatically generated. |
| } | ||
|
|
||
| /** | ||
| * result - execution reulst. |
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.
reulst -> result.
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 a lot, have fixed.
| */ | ||
| public String triggerDirectoryScanner() throws IOException { | ||
| if (reconcileRunning.get()) { | ||
| return "Trigger DirectoryScanner failed, beacause it's running. Please try again later."; |
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.
beacause -> because.
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.
have fixed. Thanks a lot ~
| "Try again later.", e); | ||
| reconcileRunning.set(false); | ||
| throw 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.
It's safer to capture the Throwable.
try {
reconcile();
} catch (Exception e) {
...
} catch (Throwable e) {
reconcileRunning.set(false);
}
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.
Nice suggestions. Have fixed it. Thanks~
|
💔 -1 overall
This message was automatically generated. |
24a42c6 to
327e937
Compare
|
💔 -1 overall
This message was automatically generated. |
327e937 to
373d9ea
Compare
|
💔 -1 overall
This message was automatically generated. |
373d9ea to
b193a44
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The failed unit tests were all passed in my local. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@ayushtkn @Hexiaoqiao @zhangshuyan0 Sir, could you please help me review this PR when you have free time? Thanks a lot~ |
tomscut
left a comment
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.
|
There is a test failure for directory scanner itself, can you check? |
| "This cycle terminating immediately because 'shouldRun' has been deactivated"); | ||
| return; | ||
| } | ||
| if (reconcileRunning.get()) { |
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.
Using the reconcileRunning variable in this way cannot prevent two threads from executing reconcile() at the same time. When this thread is running after line410 and before line416, another thread may set reconcileRunning to true.
| * @return | ||
| */ | ||
| public String triggerDirectoryScanner() throws IOException { | ||
| if (reconcileRunning.get()) { |
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.
Just as mentioned above.
|
We're closing this stale PR because it has been open for 100 days with no activity. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
Description of PR
Please see HDFS-17072.
How was this patch tested?
Add unit tests.