You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "mnpoonia (via GitHub)" <gi...@apache.org> on 2026/07/06 09:16:36 UTC
[PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
mnpoonia opened a new pull request, #8456:
URL: https://github.com/apache/hbase/pull/8456
…WithAllocationInconsistencies has a race condition between backingMapValidated and cacheState=ENABLED
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "guluo2016 (via GitHub)" <gi...@apache.org>.
guluo2016 commented on code in PR #8456:
URL: https://github.com/apache/hbase/pull/8456#discussion_r3530215432
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestRecoveryPersistentBucketCache.java:
##########
@@ -251,9 +253,12 @@ public void testBucketCacheRecoveryWithAllocationInconsistencies() throws Except
BucketCache newBucketCache = new BucketCache("file:" + testDir + "/bucket.cache", 36 * 1024,
8192, bucketSizes, writeThreads, writerQLen, testDir + "/bucket.persistence",
DEFAULT_ERROR_TOLERATION_DURATION, conf);
- while (!newBucketCache.getBackingMapValidated().get()) {
- Thread.sleep(10);
- }
+ // backingMapValidated is set inside retrieveFromFile(), but isCacheEnabled() (which
+ // getBlock() checks) is set to true only in the finally block that follows. Waiting on
+ // backingMapValidated alone leaves a race window; await both conditions together.
+ await().atMost(Duration.ofSeconds(30)).pollDelay(Duration.ofMillis(10))
+ .pollInterval(Duration.ofMillis(10)).until(
Review Comment:
For me is ok.
A small nit: pollDelay would be autimatically set as to the same value as pollInterval.
Since both are set to 10ms here, maybe the `pollDelay(Duration.ofMillis(10))`can be removed.
<img width="1101" height="335" alt="Image" src="https://github.com/user-attachments/assets/73be95d2-369a-430e-869e-c0968b518bcf" />
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "wchevreuil (via GitHub)" <gi...@apache.org>.
wchevreuil merged PR #8456:
URL: https://github.com/apache/hbase/pull/8456
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "mnpoonia (via GitHub)" <gi...@apache.org>.
mnpoonia commented on code in PR #8456:
URL: https://github.com/apache/hbase/pull/8456#discussion_r3530551672
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestRecoveryPersistentBucketCache.java:
##########
@@ -251,9 +253,12 @@ public void testBucketCacheRecoveryWithAllocationInconsistencies() throws Except
BucketCache newBucketCache = new BucketCache("file:" + testDir + "/bucket.cache", 36 * 1024,
8192, bucketSizes, writeThreads, writerQLen, testDir + "/bucket.persistence",
DEFAULT_ERROR_TOLERATION_DURATION, conf);
- while (!newBucketCache.getBackingMapValidated().get()) {
- Thread.sleep(10);
- }
+ // backingMapValidated is set inside retrieveFromFile(), but isCacheEnabled() (which
+ // getBlock() checks) is set to true only in the finally block that follows. Waiting on
+ // backingMapValidated alone leaves a race window; await both conditions together.
+ await().atMost(Duration.ofSeconds(30)).pollDelay(Duration.ofMillis(10))
+ .pollInterval(Duration.ofMillis(10)).until(
Review Comment:
You're right — Awaitility's pollDelay defaults to the same value as pollInterval when not explicitly set. So we could have written just:
await().atMost(Duration.ofSeconds(30))
.pollInterval(Duration.ofMillis(10)).until(...)
and it would have the same behavior (10ms initial delay, 10ms between polls). Setting both explicitly just makes the intent clearer. What do you 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "mnpoonia (via GitHub)" <gi...@apache.org>.
mnpoonia commented on PR #8456:
URL: https://github.com/apache/hbase/pull/8456#issuecomment-4935062833
Thank you @wchevreuil for reviewing and backporting this from master down to branch-2.6! Appreciate you shepherding this through all the branches.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "mnpoonia (via GitHub)" <gi...@apache.org>.
mnpoonia commented on PR #8456:
URL: https://github.com/apache/hbase/pull/8456#issuecomment-4893325268
Thanks @wchevreuil. i was thinking the same but then i thought how much difference will pollinterval of 10 ms and 100 ms would make.
But then adding it will make sure test behaviour would be same as before. I can add it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "mnpoonia (via GitHub)" <gi...@apache.org>.
mnpoonia commented on code in PR #8456:
URL: https://github.com/apache/hbase/pull/8456#discussion_r3530551672
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestRecoveryPersistentBucketCache.java:
##########
@@ -251,9 +253,12 @@ public void testBucketCacheRecoveryWithAllocationInconsistencies() throws Except
BucketCache newBucketCache = new BucketCache("file:" + testDir + "/bucket.cache", 36 * 1024,
8192, bucketSizes, writeThreads, writerQLen, testDir + "/bucket.persistence",
DEFAULT_ERROR_TOLERATION_DURATION, conf);
- while (!newBucketCache.getBackingMapValidated().get()) {
- Thread.sleep(10);
- }
+ // backingMapValidated is set inside retrieveFromFile(), but isCacheEnabled() (which
+ // getBlock() checks) is set to true only in the finally block that follows. Waiting on
+ // backingMapValidated alone leaves a race window; await both conditions together.
+ await().atMost(Duration.ofSeconds(30)).pollDelay(Duration.ofMillis(10))
+ .pollInterval(Duration.ofMillis(10)).until(
Review Comment:
You're right — Awaitility's pollDelay defaults to the same value as pollInterval when not explicitly set. So we could have written just:
await().atMost(Duration.ofSeconds(30))
.pollInterval(Duration.ofMillis(10)).until(...)
and it would have the same behavior (10ms initial delay, 10ms between polls). Setting both explicitly just makes the intent clearer. What do you think.
If you want it concise, we can remove pollDelay
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "guluo2016 (via GitHub)" <gi...@apache.org>.
guluo2016 commented on code in PR #8456:
URL: https://github.com/apache/hbase/pull/8456#discussion_r3532974062
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/bucket/TestRecoveryPersistentBucketCache.java:
##########
@@ -251,9 +253,12 @@ public void testBucketCacheRecoveryWithAllocationInconsistencies() throws Except
BucketCache newBucketCache = new BucketCache("file:" + testDir + "/bucket.cache", 36 * 1024,
8192, bucketSizes, writeThreads, writerQLen, testDir + "/bucket.persistence",
DEFAULT_ERROR_TOLERATION_DURATION, conf);
- while (!newBucketCache.getBackingMapValidated().get()) {
- Thread.sleep(10);
- }
+ // backingMapValidated is set inside retrieveFromFile(), but isCacheEnabled() (which
+ // getBlock() checks) is set to true only in the finally block that follows. Waiting on
+ // backingMapValidated alone leaves a race window; await both conditions together.
+ await().atMost(Duration.ofSeconds(30)).pollDelay(Duration.ofMillis(10))
+ .pollInterval(Duration.ofMillis(10)).until(
Review Comment:
> Setting both explicitly just makes the intent clearer
Agreed，thanks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery… [hbase]
Posted by "mnpoonia (via GitHub)" <gi...@apache.org>.
mnpoonia commented on PR #8456:
URL: https://github.com/apache/hbase/pull/8456#issuecomment-4893892627
@wchevreuil Added the pollDelay and pollInterval of 10ms. Please review.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org