-
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-16305.Record the remote NameNode address when the rolling log is triggered. #3629
Conversation
💔 -1 overall
This message was automatically generated. |
It looks like jenkins failed, there are some exceptions. It doesn't seem to have much to do with the pr I submitted. |
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.
Left one minor nit, otherwise looks good
@@ -413,6 +413,8 @@ private boolean tooLongSinceLastLoad() { | |||
return new MultipleNameNodeProxy<Void>() { | |||
@Override | |||
protected Void doWork() throws IOException { | |||
LOG.info("Triggering log scrolling to the remote NameNode, " + |
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.
replace log scrolling
with log rolling
?
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 log already exists on line 427
: LOG.info("Triggering log roll on remote NameNode");
.
Should we remove the original log?
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 @virajjasani for the comment and review.
Yes, I agree with @tomscut's opinion. There is no need to record logs repeatedly.
09d0cea
to
e3259be
Compare
💔 -1 overall
This message was automatically generated. |
Can you help review this pr again, @virajjasani @tomscut . |
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 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)
Thank you very much. @virajjasani @tomscut |
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
@jianghuazhu Thanks for contribution. @ayushtkn @virajjasani @tomscut Thanks for review! |
Description of PR
When StandbyNN triggers the rolling log, it sends a request to the remote NameNode. But there is no way to know the specific information of the NameNode.
Here are some log information:
We can try to record the specific address.
This is the purpose of this pr.
How was this patch tested?
Some log information has been changed here, which is not very stressful for testing.