-
Couldn't load subscription status.
- Fork 3.4k
HBASE-27390: prevent NPE when task status is null #4853
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
|
FWIW i wonder if this invalid statue occurs due to the connection bug that was in 2.5.0 and remediated in 2.5.1. I would think that could lead us to a weird spot where there is a task not fully initialized with a status. Here's how I got to this simple null check: Each task has a status The task is implemented in two spots... looking at those two spots its only needs to have a null check in one place and A monitored task status is initially null... something needs to call set status We can see here its done in many places and is just a string var |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
I've been looking into how these Tasks are created. It seems like all In terms of usages, there are a few usages of getStatus and getDescription, mostly assuming non-null (except one which does a null check). If something were to accidentally break the convention of always calling setDescription, we'd have a similar failure on our hands. Since these classes are all IA.Private, I think we should change the constructor of While here, I think we should consider adding a null check in Thoughts? |
|
@bbeaudreault That makes sense to me. I also had to update I used a precondition check for not null values as I see that's common throughout the code but can add a null check and plain return if that's better. |
|
💔 -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.
New changes look good, but I think you need to import the shaded version of Preconditions. com.google.common.base is a banned import, which is why all the pre-commit checks are failing. I'm guessing also need to run spotless too.
Can you see if there is any test class we could add a quick test of this in?
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…atus null (#4853) Signed-off-by: Bryan Beaudreault <[email protected]>
…atus null (#4853) Signed-off-by: Bryan Beaudreault <[email protected]>
…when ServerTask status null (apache#4853) Signed-off-by: Bryan Beaudreault <[email protected]>
…atus null (apache#4853) Signed-off-by: Bryan Beaudreault <[email protected]>
Simple null check to prevent the following spam when a task does not have a status.Simple changes to prevent NPE spam in hmaster logs