-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-16704. Datanode return empty response instead of NPE for GetVolumeInfo during restarting #4661
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.
I think this a behaviour change, and guess governed by two policies, One is change of public API behaviour and second change in Metrics values as well. Earlier when the storage wasn't initialised the call was failing now it will be returning an empty string. So, that is a behaviour change and can be confused with a state when data.getVolumeInfoMap()
is empty.
Extra Stuff: This log line just below is wrong, incorrect placeholder, can fix in a separate jira if interested. :-)
Line 3645 in 123d1aa
LOG.debug("Reading diskbalancer Status failed. ex:{}", ex); |
@ayushtkn Thank you very much for helping me review it. Yes, you are right, we should return an empty value for this case. How about change it same with
Developers or SREs are very sensitive to NPE, so I feel we shouldn't out put it in this expected situation.
Copy, sir. I will fix it in a separate Jira. |
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 current change makes sense to me.
If no test failures. Changes LGTM.
💔 -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. +1 from my side.
@ZanderXu Thanks for your contribution. IIRC, there is also some other precondition checks before initialized, Would you mind to give another check? such as |
@Hexiaoqiao Thanks for your review.
Sure, I will check them. BTW, If it's ok, can I fix them in a new PR? I hope this PR is just used to fix NPE in |
Are we good here? or Waiting for some more modifications? |
Yes, we are here, and I have create a new PR-4699 to fix other cases in DataNode.java. @ayushtkn @Hexiaoqiao Can you help me review them and give me a suggestion that we fix them in two PR or one? |
@Hexiaoqiao @ayushtkn Master, ping, please help me merge this PR and review PR-4699? Thanks |
💔 -1 overall
This message was automatically generated. |
…meInfo during restarting (apache#4661). Contributed by ZanderXu. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org> Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
Description of PR
During datanode starting, I found some NPE in logs:
Because the storage of datanode not yet initialized when we trying to get metrics of datanode, and related code as below:
The logic is ok, but I feel that the more reasonable logic should be return an empty response instead of NPE, because InfoServer will be started before initBlockPool.