-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29394 Possible race in BufferedMutatorOverAsyncBufferedMutator #7089
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
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.
Pull Request Overview
This PR addresses a potential race condition in BufferedMutatorOverAsyncBufferedMutator by adding a synchronized block within the asynchronous callback and by introducing a targeted test case to validate the race scenario.
- Introduces a synchronized block in the callback of BufferedMutatorOverAsyncBufferedMutator to update shared state safely.
- Adds TestBufferedMutatorOverAsyncBufferedMutator.java to simulate and verify the race condition behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BufferedMutatorOverAsyncBufferedMutator.java | Synchronizes listener callback to prevent race conditions. |
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestBufferedMutatorOverAsyncBufferedMutator.java | Adds a race condition test to validate correct exception handling and flush behavior. |
...nt/src/main/java/org/apache/hadoop/hbase/client/BufferedMutatorOverAsyncBufferedMutator.java
Show resolved
Hide resolved
The key thing here is that, for a failed mutation, in the callback in mutate method, we remove the future from futures, and then, before we add the exception to errors, internalFlush method is called and finished, so in the method, we can not get the future since it has already been removed from the futures field, and also we can not get the exception from the errors field since it has not been added to the errors field yet. The solution is to also use synchronized(this) in the callback. |
}); | ||
mockedPair.when(() -> Pair.newPair(any(), any())).then(i -> { | ||
beforeFlush.countDown(); | ||
afterFlush.await(5, TimeUnit.SECONDS); |
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.
Here we need to use timed await because after the fixing in this PR, the above flush operation can not finish before we leave the synchronized block, so after fixing finally we will timeout here and fall through.
Before the fix, the await will return soon and then the test will fail.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.