You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/02/04 03:04:09 UTC
[GitHub] [hbase] markrmiller opened a new pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
markrmiller opened a new pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120
…s a very expensive array.
Here is a first pass, says it can't merge for some reason so I will be back for draft 2.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r378481396
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
Review comment:
Yeah, so AtomicLongArray to ensure we see an up to date value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r375011027
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -34,7 +36,7 @@
public static final HBaseClassTestRule CLASS_RULE =
HBaseClassTestRule.forClass(TestSyncTimeRangeTracker.class);
- private static final int NUM_KEYS = 10000000;
+ private static final int NUM_KEYS = 1000000;
Review comment:
So this is the actual fix right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache9 commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r375010910
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
Review comment:
But this array will only be accessed by one thread? Why do we need to use AtomicLongArray?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r374785224
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
+ private long min = Long.MAX_VALUE; // effectively final
+ private long max = 0; // effectively final
public RandomTestData() {
if (ThreadLocalRandom.current().nextInt(NUM_OF_THREADS) % 2 == 0) {
for (int i = 0; i < NUM_KEYS; i++) {
- keys[i] = i + ThreadLocalRandom.current().nextLong(NUM_OF_THREADS);
- if (keys[i] < min) min = keys[i];
- if (keys[i] > max) max = keys[i];
+ keys.set(i, i + ThreadLocalRandom.current().nextLong(NUM_OF_THREADS));
+ if (keys.get(i) < min) min = keys.get(i);
Review comment:
See your checkstyle. Needs parents around the 'min = key[i];'
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#issuecomment-581997764
Your branch up-to-date Mr. Miller?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] markrmiller edited a comment on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
markrmiller edited a comment on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#issuecomment-585396366
Sorry, been chasing a local clean test run.
The AtomicLongArray was needed for these tests to pass more regularly for me to ensure we always look at an up to date value.
The memory issue seemed wasteful and a bit off given the goals of the test and also counter productive to running test like this in parallel.
I've updated my pr.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] busbey commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
busbey commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r375495316
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
+ private long min = Long.MAX_VALUE; // effectively final
+ private long max = 0; // effectively final
Review comment:
they get assigned like right below here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r378480422
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
Review comment:
Basically I ran this test a lot before and after these changes to try and confirm a fix and this is what seemed to work.
Technically, even if a single thread accesses a data structure, if it is a different thread than the one that created it, you still need to publish objects correctly for multiple threads - concurrency is not required. So they need a mem barrier or to be final or to be effectively final or a 32-bit primitive(I think?).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r374784868
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
+ private long min = Long.MAX_VALUE; // effectively final
+ private long max = 0; // effectively final
Review comment:
Don't want to make them final?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
saintstack commented on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#issuecomment-584255168
@markrmiller See comments above?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r374785836
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
Review comment:
This is the 'fix'? Using atomic array. Sounds good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r378482230
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -34,7 +36,7 @@
public static final HBaseClassTestRule CLASS_RULE =
HBaseClassTestRule.forClass(TestSyncTimeRangeTracker.class);
- private static final int NUM_KEYS = 10000000;
+ private static final int NUM_KEYS = 1000000;
Review comment:
This is kind of separate. I noticed this huge array took a lot of RAM and it did not seem necessary.
I also noticed that the array is so large that the threads working on it don't overlap much as is.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#discussion_r378482606
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSyncTimeRangeTracker.java
##########
@@ -84,23 +86,23 @@ public void run() {
assertTrue(trr.getMin() == 0);
}
- static class RandomTestData {
- private long[] keys = new long[NUM_KEYS];
- private long min = Long.MAX_VALUE;
- private long max = 0;
+ static class RandomTestData {
+ private final AtomicLongArray keys = new AtomicLongArray(NUM_KEYS);
+ private long min = Long.MAX_VALUE; // effectively final
+ private long max = 0; // effectively final
Review comment:
Right, would be ideal to make them final, settling for marking them effectively final.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] markrmiller commented on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
Posted by GitBox <gi...@apache.org>.
markrmiller commented on issue #1120: HBASE-23787: TestSyncTimeRangeTracker fails quite easily and allocate…
URL: https://github.com/apache/hbase/pull/1120#issuecomment-585396366
Sorry, been chasing a local clean test run.
The AtomicLongArray was need for these tests to pass more regularly for me to ensure we always look at an up to date value.
The memory issue seemed wasteful and a bit off given the goals of the test and also counter productive to running test like this in parallel.
I've updated my pr.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services