HBASE-24174 Fix findbugs warning in branch-1#1494
Merged
Reidddddd merged 1 commit intoapache:branch-1from Apr 17, 2020
Merged
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
@Reidddddd in branch-2+, this has been suppressed with as part of HBASE-20069 |
Reidddddd
force-pushed
the
HBASE-24174
branch
2 times, most recently
from
fec95bc to
9ebc179
Compare
virajjasani
reviewed
Apr 13, 2020
|
|
||
| @Override | ||
| public void onConfigurationChange(Configuration newConf) { | ||
| public synchronized void onConfigurationChange(Configuration newConf) { |
Contributor
There was a problem hiding this comment.
If we want to make this change, it should go to master, branch-2 first right?
Contributor
Author
There was a problem hiding this comment.
I saw the warning in branch-1 only after HBASE-24174. I can forward-port to branch-2 and master. But as you said it is supressed, dilemma...
Contributor
There was a problem hiding this comment.
Right, it is suppressed there. Either we can backport suppressed one or forward port this to branch-2+ and remove suppression there. Sure, let's see QA 👍
Contributor
Author
|
Let's see if the warning fade, after this commit.. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Let me check how does the branch-2 suppress the warning...( |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
The suppress doesn't work?... o_0 |
Contributor
Oh ok, it seems we will have to put it over where we define authManager. Interesting! |
Contributor
Author
Tried, the previous commit... |
Contributor
Author
|
the 2nd commit suppressed on definition |
Contributor
oops, missed previous commit. This is weird, how come it is not getting suppressed on branch-1 |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
so buggy........having no clue.. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
I just found the findbug's line number doesn't match. |
Contributor
Oh, where can we see that? Are you referring to this one: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-1494/8/artifact/out/branch-findbugs-hbase-server-warnings.xml ? |
Contributor
yeah, agree... |
Contributor
Author
|
This 2270 and 2271 line is the original code without patch. |
Contributor
wow, then it's not even looking in the patch, weird... |
Contributor
|
that's the findbugs report for the branch, before the patch. the report for after the patch says there is no issue. so it's correctly reporting exactly what you're trying to fix here. |
Contributor
(I am purposefully only commenting on what qabot is saying, not on wether or not ignoring this warning is a good idea.) |
Contributor
Author
|
Thanks @busbey ! Oh, there're two findbugs..., oversight. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Rebased branch-1 |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
Yup, we are good :) |
virajjasani
reviewed
Apr 17, 2020
| System.setProperty("hadoop.policy.file", "hbase-policy.xml"); | ||
| synchronized (authManager) { | ||
| authManager.refresh(newConf, new HBasePolicyProvider()); | ||
| if (authorize) { |
Contributor
There was a problem hiding this comment.
btw, this if condition is an enhancement for this patch? is it necessary?
Contributor
Author
There was a problem hiding this comment.
Yes, it is, to avoid unnecessary refresh. If authorization is disabled, then no need to refresh it which it is time consuming.
We should forward-port this patch to branch-2+ and master as well, quite some changes.
Contributor
Author
|
Thanks @virajjasani , we finally get here. |
asfgit
pushed a commit
that referenced
this pull request
Apr 17, 2020
Signed-off-by: Viraj Jasani <vjasani@apache.org>
Reidddddd
added a commit
to Reidddddd/hbase
that referenced
this pull request
Apr 22, 2020
…he#1494) Signed-off-by: Viraj Jasani <vjasani@apache.org>
udaynpusa
pushed a commit
to mapr/hbase
that referenced
this pull request
Jan 30, 2024
…he#1494) Signed-off-by: Viraj Jasani <vjasani@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.