HBASE-26555 Fix findbugs/spotbugs findings #3936
Open
+256
−220
Conversation
This file contains 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
apurtell
force-pushed
the
HBASE-26555
branch
2 times, most recently
from
843ea7e to
c2dc14b
Compare
|
🎊 +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. |
|
Fix LoadTestKVGenerator related failures. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Dec 13, 2021
| private final long clientId; | ||
|
|
||
| private Random rng; |
There was a problem hiding this comment.
I do not see any real differences here? What is the spotbugs issue here?
There was a problem hiding this comment.
Spotbugs wants you to allocate a Random object and reuse it as often as possible. This is because if you do not reuse Random, if you just do like so in a method
Random r = new Random();
// do something with 'r'
the quality of the generated random numbers is poor.
There was a problem hiding this comment.
I would say let's just use ThreadLocalRandom, unless SecureRandom is required, then we could create a static SecureRandom instance and use it.
| // Ensure priority is a positive integer | ||
| int priority = className.hashCode(); | ||
| if (priority < 0) { | ||
| priority = -priority; |
There was a problem hiding this comment.
This is not the correct fix... You are just repeating the implementation of Math.abs here...
If priority is Integer.MIN_VALUE, then -priority will still be Integer.MIN_VALUE...
There was a problem hiding this comment.
Thanks for pointing that out. As explained by the help text for the spotbugs finding the issue is Math.abs(INT_MIN) can return INT_MIN. So if ABS is supposed to ensure a positive number, it will not always do so. The fix is to change the sign of the value if it is negative and should not be negative. Needs more here, then.
There was a problem hiding this comment.
The main problem is not Math.abs, is INT_MIN. As the absolute value of INT_MIN is greater than INT_MAX, there is no positive int value can represent the absolute value of INT_MIN... You could try the piece of code here, let priority to be INT_MIN, you will still get a INT_MIN with -priority...
| byte[] testBytes = new byte[10]; | ||
| rand.nextBytes(testBytes); | ||
| RNG.nextBytes(testBytes); |
There was a problem hiding this comment.
What is the real gain here? We just the the random once, and I suggest that we just use ThreadLocalRandom instead...
There was a problem hiding this comment.
Spotbugs wants you to allocate a Random object and reuse it as often as possible. This is because if you do not reuse Random, if you just do like so in a method
Random r = new Random();
// do something with 'r'
the quality of the generated random numbers is poor.
There was a problem hiding this comment.
Sure, let's use ThreadLocalRandom.current() instead of the static Random instance.
| for (int i = 0; i < NELEM; ++i) { | ||
| int key = rand.nextInt(MAX_KEY); | ||
| int key = RNG.nextInt(MAX_KEY); |
There was a problem hiding this comment.
Ditto, since we do not have a fixed seed here, let's just use ThreadLocalRandom.
There was a problem hiding this comment.
I don't care really the objective is reuse of the Random object
| assertEquals(0, props.size()); | ||
| testBase.loadMonkeyProperties(props, conf); | ||
| new IntegrationTestDDLMasterFailover(); |
There was a problem hiding this comment.
Why a new without assignment?
There was a problem hiding this comment.
Because the assignment is flagged as a dead store. This code still works. The constructor is called. The warning about a dead store is avoided. It is a trivial issue and I don't care much either way.
There was a problem hiding this comment.
Then do we really need to create this instance? It has other side effects?
| long chainId = Math.abs(new Random().nextLong()); | ||
| // ensure chainId is positive | ||
| long chainId = RNG.nextLong(); | ||
| if (chainId < 0) { |
There was a problem hiding this comment.
Ditto, not the correct way for fixing the abs problem...
|
Each module has its own commit in the PR branch @Apache9 . I think that is the best we can do as far as keeping each review item small. In other words I am aware the patch is big over all. So there are multiple commits on this PR. Find them here: https://github.com/apache/hbase/pull/3936/commits Each commit has the changes just for one maven module. |
We could just use multiple PRs to get them in? A problem for a big PR is that, we could have a lot of small nits every time when reviewing, on different part of the big PR, then we can not land the whole PR, which will lead to a very long review time and easy to introduce conflicts with other commits... |
You want me to close this and open multiple PRs? How many PRs? How would you divide this? One PR per module? One PR per file? There are not THAT many changes here. I think it is not the hassle you describe. I would like to contribute spotbugs changes but it is hardly a priority relative to other things. This PR is ready. I hope it can be used. I do not plan to open multiple PRs for this. If that is a problem, feel free to just close this and resolve the JIRA. |
|
I am going to split the Random changes into a separate JIRA and PR because that part has taken on a life of its own. Edit: It is HBASE-26582 |
apurtell
force-pushed
the
HBASE-26555
branch
from
c80ecfd to
7eba7d7
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
joshelser
approved these changes
Jan 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Commits in the PR branch break down changes per module.