Skip to content

HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery…#8456

Open
mnpoonia wants to merge 1 commit into
apache:masterfrom
mnpoonia:HBASE-30275
Open

HBASE-30275 TestRecoveryPersistentBucketCache#testBucketCacheRecovery…#8456
mnpoonia wants to merge 1 commit into
apache:masterfrom
mnpoonia:HBASE-30275

Conversation

@mnpoonia

@mnpoonia mnpoonia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

…WithAllocationInconsistencies has a race condition between backingMapValidated and cacheState=ENABLED

@wchevreuil wchevreuil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this fix.

Just a nit comment: should we set pollDelay and pollInterval to 10ms? IIUC, Awaitility sets these to 100ms, by default.

@mnpoonia

mnpoonia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

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 Now i see that adding it will make sure test behaviour would be same as before. I will add it.

@mnpoonia

mnpoonia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@wchevreuil Added the pollDelay and pollInterval of 10ms. Please review.

// 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(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Image

@mnpoonia mnpoonia Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting both explicitly just makes the intent clearer

Agreed,thanks

…WithAllocationInconsistencies has a race condition between backingMapValidated and cacheState=ENABLED
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants