-
Notifications
You must be signed in to change notification settings - Fork 587
HDDS-10151. Replace single-use Random objects with RandomUtils in test classes #6041
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
adoroszlai
left a comment
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 @will-sh for the patch. The change mostly looks good, see two items inline.
| final byte[] bytes = new byte[1 << 10]; | ||
| for (int i = 0; i < 1000; i++) { | ||
| random.nextBytes(bytes); | ||
| checkBytes(bytes, random.nextInt(bytes.length)); | ||
| RandomUtils.nextBytes(bytes.length); | ||
| checkBytes(bytes, RandomUtils.nextInt(0, bytes.length)); |
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.
nextBytes returns the byte array, which is ignored here. So bytes is the same in each iteration instead of having random content.
Since this results in a new array in each iteration, we can remove the original bytes array.
final int len = 1 << 10;
for (int i = 0; i < 1000; i++) {
checkBytes(RandomUtils.nextBytes(len), RandomUtils.nextInt(0, len));
}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 the suggestion, it is corrected.
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Random; | ||
| import org.apache.commons.lang3.RandomUtils; |
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.
nit: please try to keep alphabetical order, move to:
Lines 20 to 21 in a7d4b06
| import com.google.common.collect.ImmutableSet; | |
| import org.apache.hadoop.hdds.client.ECReplicationConfig; |
(the same minor issue applies to some of the other files)
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.
Done.
|
Thanks @will-sh for updating the patch. |
|
Thanks for the review @adoroszlai |
What changes were proposed in this pull request?
HDDS-10151: Replace single-use Random objects with RandomUtils in test classes
Please describe your PR in detail:
Replace single-use Random objects by RandomUtils or similar. This is a code optimization task aimed at improving the efficiency of the codebase.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10151
How was this patch tested?
Each test class are manually tested using
mvn test -Dtest=ClassNameAll the tests passed after the code change.
For the Abstract classes, tests passed for the sub-classes that inherit from the superclasses after code change, for example:
TestOzoneContractLegacy extends AbstractOzoneContractTest extends AbstractContractSeekTest
TestOFSWithOMRatis extends AbstractRootedOzoneFileSystemTest
Note
RandomUtils cannot directly set a seed.
In the below test classes (TestDeletedBlockLog.java, TestLeaderChoosePolicy.java, TestDeletedBlocksTxnShell.java)
We don't see much benefit to set a seed, so Random is replaced by RandomUtils
The test class TestMultipartObjectGet.java also doesn't really need to be secure so the SecureRandom() is replaced by RandomUtils