-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28174 #5501
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-28174 #5501
Conversation
…s in REST API URLs)
|
💔 -1 overall
This message was automatically generated. |
hbase-rest/src/main/java/org/apache/hadoop/hbase/rest/RowSpec.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
…hese weren't picked up by the build).
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Support both plain and URL-safe base64 encoding (when not specified, assume url-safe since this data is expected to be supplied in a URL).
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
left a 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.
Thanks for submitting this, @judilsteve. Overall, lgtm.
One nit comment: I see that the encoding param drives encoding for both rowkey and columns qualifiers, so should we call it "Encoding", rather than "Key-Encoding"?
Also, is it possible to add some unit tests? You can add extra testing methods for the encoding in the existing test classes.
|
Actioned. I've never used JUnit before, so the test code might not be particularly idiomatic. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Test failure log: It's a similar stack trace for the second test failure. I'm not able to repro this on my machine, will do some Googling. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
That's a bit more of a clue. Will keep digging tomorrow. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks for providing UTs, @judilsteve ! One last ask I have before merging this is to also update the ref guide to explain this extra param. You can find the REST section under |
…so adds documentation for some previously undocumented endpoints).
|
Documentation has been added. The base DELETE and multi-get endpoints were not documented at all, so I added examples for both the plaintext and base64 encoded versions. The examples are not exhaustive, but readers can combine multiple examples to get what they need (e.g. there is no example for placing a row-level delete marker with base64 encoding, but there is an example for deleting an entire row via plaintext row-key and an example for deleting a specific column via a base64-encoded row-key/column). |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@wchevreuil I am not sure what to do about the test failure above. The logs show a timeout, which makes me suspicious of an issue on the build server, since none of the previous builds had this problem. The failing test is listed as "org.apache.hadoop.hbase.TestAcidGuaranteesWithBasicPolicy.(?)" which is even weirder. I ran |
wchevreuil
left a 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.
LGTM, +1. The UT failure looks unrelated to this change.
See https://issues.apache.org/jira/browse/HBASE-28174
Very much a draft. I'm yet to test this. Testing checklist (hopefully I've caught everything here):