-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-24752 NPE/500 accessing webui on master startup #2148
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.
Looks good, left one comment.
@@ -224,6 +225,11 @@ AssignmentManager assignmentManager = master.getAssignmentManager(); | |||
<& AssignmentManagerStatusTmpl; assignmentManager=master.getAssignmentManager()&> | |||
</%if> | |||
<%if !master.isInMaintenanceMode() %> | |||
<%java> | |||
while (master.getMasterCoprocessorHost() == null) { | |||
Threads.sleep(50); |
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.
Let's also print some message which we can removed after getting out of this while
loop?
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 researched it and it seems I can’t
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.
Would the whole page not load (display) when master.getMasterCoprocessorHost() == null ?
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.
Good question, the only concern is what will the page display when master.getMasterCoprocessorHost()
is null?
Is this situation reproable by any chance? If not, it's fine, this fix looks good anyways.
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.
As of commit 82d0990 in master branch, I don't see the master.getMasterCoprocessorHost().findCoprocessor() call below.
One option is to find the commit which removed master.getMasterCoprocessorHost().findCoprocessor() call.
Another option is to use code similar to existing one:
<td><% master.getMasterCoprocessorHost() == null ? "[]" :
java.util.Arrays.toString(master.getMasterCoprocessors()) %></td>
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.
my negligence, there are some similar problems and solutions in front
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -223,7 +223,7 @@ AssignmentManager assignmentManager = master.getAssignmentManager(); | |||
<%if master.getAssignmentManager() != null %> | |||
<& AssignmentManagerStatusTmpl; assignmentManager=master.getAssignmentManager()&> | |||
</%if> | |||
<%if !master.isInMaintenanceMode() %> | |||
<%if !master.isInMaintenanceMode() && master.getMasterCoprocessorHost() != null %> |
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
nit: double space between && and 'master'
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 for reminding
🎊 +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.
+1
+1 |
Pending QA of latest build. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Closes #2148 Signed-off-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Closes #2148 Signed-off-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Closes #2148 Signed-off-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Closes apache#2148 Signed-off-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Closes apache#2148 Signed-off-by: Ted Yu <yuzhihong@gmail.com> Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 6fba086) Change-Id: I95ae9e133877ed30adcb923b6fb3780136171bfc
No description provided.