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-25299 Scan#setRowPrefixFilter Unexpected behavior #2674
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@infraio Hi, could u help me to review this patch? |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java
Outdated
Show resolved
Hide resolved
c8fcfda
to
c55dd55
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Looks good to me, but let's wait for what people say from dev mailing list. |
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Scan.java
Outdated
Show resolved
Hide resolved
* Users can compose this logic externally through | ||
* {@link ClientUtil#calculateTheClosestNextRowKeyForPrefix(byte[])}, | ||
* so there is no need to implement it inside. |
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.
ClientUtil
is IA.Private, hence not really recommended for direct use by downstreamers.
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.
ClientUtil
is IA.Private, hence not really recommended for direct use by downstreamers.
@huaxiangsun suggest me to give more detail, so i put this link. What i mean is just user can do this things by themself. Should i remove this link?
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 can remove just this statement:
* Users can compose this logic externally through
* {@link ClientUtil#calculateTheClosestNextRowKeyForPrefix(byte[])},
* so there is no need to implement it inside.
because ClientUtil
is IA.Private, hence we should not recommend it in Javadoc.
@huaxiangsun Thought?
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.
- Users can compose this logic externally through
- {@link ClientUtil#calculateTheClosestNextRowKeyForPrefix(byte[])},
- so there is no need to implement it inside.
+1 for remove these comment.
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.
+1 to remove. Sorry, it is just I did not know the alternative way to archive the goal.
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.
Yeah, this is a tough one. Ideally we can provide the link in comment but the class here is IA.Private.
@thangTang can you please also write a unit test with the same scenarios that you mentioned over Jira? i.e |
I'd like to, but should i write the UT in the patch? I think it would be failure and the CI -1. |
I didn't get why it would fail. What I mean to say is you can write unit test such that the condition you provided with "111" and "112" holds true. Please let me know if you still have doubts :) |
c55dd55
to
eec82f2
Compare
UT updated. |
🎊 +1 overall
This message was automatically generated. |
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.
+1 overall, let's wait for @huaxiangsun 's response. Since ClientUtil is IA.Private, maybe it's not good to provide it's reference in Javadoc. However, I am fine to go with consensus.
Thanks @thangTang
* Users can compose this logic externally through | ||
* {@link ClientUtil#calculateTheClosestNextRowKeyForPrefix(byte[])}, | ||
* so there is no need to implement it inside. |
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 can remove just this statement:
* Users can compose this logic externally through
* {@link ClientUtil#calculateTheClosestNextRowKeyForPrefix(byte[])},
* so there is no need to implement it inside.
because ClientUtil
is IA.Private, hence we should not recommend it in Javadoc.
@huaxiangsun Thought?
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
eec82f2
to
ac44866
Compare
🎊 +1 overall
This message was automatically generated. |
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.
+1
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.