-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-29013 Make PerformanceEvaluation support larger data sets (branch-2) #6559
Conversation
Use 8-byte long integers in the code to prevent integer overflows. Signed-off-by: Duo Zhang <zhangduo@apache.org> Reviewed-by: Peng Lu <lupeng_nwpu@qq.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java
Show resolved
Hide resolved
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java
Show resolved
Hide resolved
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.
Let's change to use ThreadLocalRandom.
And also please open an addendum PR against master branch too, so we can align the code with branch-2.
@@ -1149,7 +1149,7 @@ private static long nextRandomSeed() { | |||
return randomSeed.nextLong(); | |||
} | |||
|
|||
private final int everyN; | |||
private final long everyN; | |||
|
|||
protected final Random rand = new Random(nextRandomSeed()); |
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.
So we can remove this now?
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.
Yes, we can. I didn't remove it in the first place because I wanted to minimize the code change, but now that we agree on using ThreadLocalRandom, let me clean this up.
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.
On the second thought, we can keep this member and just set it to ThreadLocalRandom.current()
to minimize the required code change. TestBase
instances are created on the fly by each thread in the pool (TestClient-%s
), so I think it's safe to do it this way. What do you think?
diff --git a/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java b/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java
index ffe33a9e8b..7bbccbd48d 100644
--- a/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java
+++ b/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java
@@ -1141,17 +1141,9 @@ public class PerformanceEvaluation extends Configured implements Tool {
* A test. Subclass to particularize what happens per row.
*/
static abstract class TestBase {
- // Below is make it so when Tests are all running in the one
- // jvm, that they each have a differently seeded Random.
- private static final Random randomSeed = new Random(EnvironmentEdgeManager.currentTime());
-
- private static long nextRandomSeed() {
- return randomSeed.nextLong();
- }
-
private final long everyN;
- protected final Random rand = new Random(nextRandomSeed());
+ protected final Random rand = ThreadLocalRandom.current();
protected final Configuration conf;
protected final TestOptions opts;
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.
to minimize the required code change
generateData
is used in other places, but without a custom seed value.
Lines 508 to 518 in fd11b9b
private byte[][] generateRandomStartKeys(int numKeys) { | |
Random random = new Random(); | |
byte[][] ret = new byte[numKeys][]; | |
// first region start key is always empty | |
ret[0] = HConstants.EMPTY_BYTE_ARRAY; | |
for (int i = 1; i < numKeys; i++) { | |
ret[i] = | |
PerformanceEvaluation.generateData(random, PerformanceEvaluation.DEFAULT_VALUE_LENGTH); | |
} | |
return ret; | |
} |
hbase/hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat2.java
Lines 545 to 555 in b7def4f
private byte[][] generateRandomStartKeys(int numKeys) { | |
Random random = ThreadLocalRandom.current(); | |
byte[][] ret = new byte[numKeys][]; | |
// first region start key is always empty | |
ret[0] = HConstants.EMPTY_BYTE_ARRAY; | |
for (int i = 1; i < numKeys; i++) { | |
ret[i] = | |
PerformanceEvaluation.generateData(random, PerformanceEvaluation.DEFAULT_VALUE_LENGTH); | |
} | |
return ret; | |
} |
So it's actually trivial to remove the Random parameter altogether.
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.
🎊 +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. |
Cherry-picked 6ebd48e and replaced
Random#nextLong(bound)
which is not available on JDK 8 withThreadLocalRandom#nextLong(bound)
(e140ae7).Not being able to provide a seed doesn't matter in this context, so this should be good enough.