-
Notifications
You must be signed in to change notification settings - Fork 157
Merge DataPlatform Java Client #612
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
…text in 2i queries
… full bucket read
…ntroduce workaround for Full Bucket Reads without providing any keys
… any coverage entry with ip '0.0.0.0'.
…lder#withReplaceCoverageContext method
…table/alternative Riak Nodes
| ``` | ||
| git clone https://github.com/basho/riak_pb | ||
| git checkout java-2.1.1.0 | ||
| git checkout 2.1.2.0 |
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.
We include riak_pb as a submodule now, so we can remove this step once that develop is merged back to this branch.
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'll rebase from the latest develop, that should resolve this
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.
@srgg I have a branch that does that - There were some changes to the TSQuery command since we switched to term-to-binary support. https://github.com/basho/riak-java-client/tree/merge-dpclient-ts1.3
We need a test around using a coverage context with a TS query request. I need to see if I wrote this line correctly:
riak-java-client/src/main/java/com/basho/riak/client/core/codec/TermToBinaryCodec.java
Line 82 in bd6cf99
| os.write(coverageContext); |
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.
We would need to add a test similar to this one, that adds a context, to make sure it encodes correctly: https://github.com/basho/riak-java-client/blob/merge-dpclient-ts1.3/src/test/java/com/basho/riak/client/core/codec/TermToBinaryCodecTest.java#L133
I think I added everything else in that branch to add the coverage context to the TTB TS Queries though.
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.
After merging1.3 branch into this one, @aleksey-suprun can prove this conversion by running (or extending, if needed) our Full Bucket Read Test
|
Only other feedback is to use adjust the formatting for braces to be on a new line by themselves, to match the rest of the codebase: I should probably document our "standard" somewhere :) |
|
Just to document the list of everything:
|
Merge the changes we did for TS 1.3 into dpclient branch, before merging back to develop.
|
Thank you @srgg and @aleksey-suprun! |
|
This PR was reintroduced as https://github.com/basho/riak-java-client/compare/merge-dpclient-rebased |
Ready for review