-
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-27369 BufferedMutator#mutate supports mutation type verification #4780
base: master
Are you sure you want to change the base?
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@carp84 sir. Could you take a look? Thanks. |
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.
I think we should add the check in AsyncBufferedMutator? BufferedMutator is just a delegation of AsyncBufferedMutator.
|
But finally they will go into the mutate(List) method? We just need to add checks there? |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
|
According to the call chain, the check logic should be added to the AsyncBufferedMutatorImpl#mutate(List) method. However, AsyncBufferedMutatorImpl#mutate(List) is a public api, and there is no description of the mutate type restriction in the comments. Besides, AsyncBufferedMutatorImpl#mutate(List) will call the AsyncTable#batch(List) method, which supports the mutation types including Get, Put, Delete, Increment, Append, and RowMutations, so we should not add the check logic to the AsyncBufferedMutatorImpl#mutate(List). |
The problem is about the API design. Increment and Append have return values but the current return values of BufferMutator methods do not return them... |
@Apache9 Sir. Do you still suggest adding check logic to AsyncBufferedMutatorImpl#mutate(List)? |
We'd better start a discssion thread on the dev list to discuss this. |
The logic of this part is really a bit confusing, but I may not be able to describe the relevant issues clearly, so I can't effectively start a discussion. |
Then this could also be thing we should discuss :) |
@Apache9 sir. I get it. I will start a discussion. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
https://issues.apache.org/jira/browse/HBASE-27369