-
Notifications
You must be signed in to change notification settings - Fork 237
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
Address spotbugs issues with TableConfigCache code #897
Address spotbugs issues with TableConfigCache code #897
Conversation
|
||
public TableConfigCache(Configuration conf) { | ||
super(conf); | ||
} | ||
|
||
@SuppressFBWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD") |
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 seems like this was the best action to take here - it was complaining that we were writing to a static field from a non-static method. Perhaps there's another way to do this that would not result in a warning?
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'll investigate this when I get back. Approving for now. Long term we shouldn't need this suppression.
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 like configMap
is static but we're setting it in a non-static method. The method actually has no dependency on any instance state. Seems like the fix might be to make this method static.
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.
@jschmidt10 - I suspected the same thing but was going to leave the design up to @hlgp.
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.
Sounds good!
|
||
public TableConfigCache(Configuration conf) { | ||
super(conf); | ||
} | ||
|
||
@SuppressFBWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD") |
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'll investigate this when I get back. Approving for now. Long term we shouldn't need this suppression.
…s than requested page size, update status
…page less than requested page size, update status" This reverts commit 76f0d0f.
No description provided.