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

CQL storage backend #166

Merged
merged 8 commits into from May 21, 2017

Conversation

@pkendall64
Contributor

pkendall64 commented Mar 15, 2017

Adds a CQL stotage backend that is compatible with the table structure of the Astynanx code.
This PR addresses issue #35.

A new library (javaslang) is introduced which provides very nice functional library which makes the code a lot more concise and understandable.

@janusgraph-bot

This comment has been minimized.

janusgraph-bot commented Mar 15, 2017

Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.

@pkendall64 pkendall64 force-pushed the orionhealth:feature/cql-backend branch from 776425e to 8fb4fe1 Mar 15, 2017

@janusgraph-bot janusgraph-bot added cla: yes and removed cla: no labels Mar 15, 2017

@mbrukman mbrukman requested a review from fppt Mar 16, 2017

@mbrukman

Thank you for the PR! This is a massive contribution. Please also add yourselves to CONTRIBUTORS.txt and your company to AUTHORS.txt in the root of this repo.

I am far from a Cassandra/CQL expert (so I'm hoping @fppt can weigh in on this, and anyone else who's proficient with Cassandra), but I'd like to ask a few things:

  • this repo uses a 4-space indent, this PR uses tabs exclusively; would it be possible to switch to 4-space indent everywhere so we don't diverge from a single standard?
  • can you please add the module to .travis.yml at the root of the repo and mark it optional so that it gets automatically tested? Then we can also see what Travis has to say about this change (I'm guessing it wasn't tested with the PR, as we specifically address each module in a separate build).
@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented Mar 16, 2017

We'll get onto those changes and update the PR, thanks for the feedback.

@pkendall64 pkendall64 closed this Mar 16, 2017

@pkendall64 pkendall64 deleted the orionhealth:feature/cql-backend branch Mar 16, 2017

@mbrukman

This comment has been minimized.

Member

mbrukman commented Mar 16, 2017

@pkendall64 – why did you delete the branch and close the PR? You can just update the code on your side and git push -f to update the branch and we'll continue the review. No need to open a new PR each time we have a round of reviews.

You can probably re-open this PR (since you created it), recreate the branch and force-push to it via git push -f and we can continue where we left off.

@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented Mar 16, 2017

Yeah, thats what I did and it just disappeared!

@mbrukman

This comment has been minimized.

Member

mbrukman commented Mar 16, 2017

@pkendall64 – you pushed "delete branch" button on GitHub, so it deleted the branch in orionhealth:feature/cql-backend but presumably you have a local copy of it on your workstation? In which case, you can just push to the same repo / branch to continue.

You can also fetch PR on the CLI using git.

In the worst case scenario, if you've deleted the branch in the orionhealth repo as well as locally, you can still attach .patch or .diff to the PR URL to get the full contents — click on either of those links to see what I mean.

@pkendall64 pkendall64 reopened this Mar 16, 2017

@pkendall64 pkendall64 force-pushed the orionhealth:feature/cql-backend branch from 8fb4fe1 to b5b4ae5 Mar 16, 2017

@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented Mar 16, 2017

Sorry about that, all fixed now. I had to checkout the old hash and force push that, then force push the new stuff over the top and now it's ok again.
Don't really know what happened the first time!

@mbrukman mbrukman requested review from hsaputra, amcp and dylanht Mar 16, 2017

@mbrukman

This comment has been minimized.

Member

mbrukman commented Mar 16, 2017

Great to hear your PR is back, @pkendall64! :-)

@amcp

good contribution. I think you need to add some more tests, there is some hairy logic in store manager regarding computing supported feature set. perhaps find a way to test enabled/disabled of features that depend on configuration.

@@ -0,0 +1,32 @@
<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

@amcp

amcp Mar 18, 2017

is this an eclipse specific file? should we be committing this to the repo, or should people be able to determine their own IDE configurations?

This comment has been minimized.

@pkendall64

pkendall64 Mar 19, 2017

Contributor

Sure, I'll remove the eclipse files

@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

@amcp

amcp Mar 18, 2017

same as above, eclipse specific

</plugins>
</pluginManagement>
</build>
</project>

This comment has been minimized.

@amcp

amcp Mar 18, 2017

add newline to the end of file

package org.janusgraph.diskstorage.cql;
import static com.datastax.driver.core.querybuilder.QueryBuilder.*;

This comment has been minimized.

@amcp

amcp Mar 18, 2017

please import specific things and not wildcard

this.tableName = tableName;
this.getter = new Getter(storeManager.getMetaDataSchema(this.tableName));
initialiseTable(this.session, this.storeManager.getKeyspaceName(), tableName, configuration);

This comment has been minimized.

@amcp

amcp Mar 18, 2017

spelled initialized elsewhere in this project so please match existing spelling

}
private Cluster initialiseCluster() throws PermanentBackendException {
// XXX: Deal with retry policy and potentially others

This comment has been minimized.

@amcp
fb.keyOrdered(false).orderedScan(false).unorderedScan(true);
break;
}
case "org.apache.cassandra.dht.ByteOrderedPartitioner": {

This comment has been minimized.

@amcp

amcp Mar 18, 2017

create string constants that get the canonical name from the actual classes. that way if version update changes the package of ByteOrderedPartitioner, you can catch it as compile time error here

This comment has been minimized.

@pkendall64

pkendall64 Mar 19, 2017

Contributor

I don't think you want to have compile time dependencies on these classes as they are internals to cassandra and we really only want this depend on the driver jar and not the cassandra internals.
Perhaps it would be better to get the simplename of the class and switch on that accordingly?

This comment has been minimized.

@amcp

amcp Apr 4, 2017

yes switch on simplename please

return builder.build();
}
private Session initialiseSession(@SuppressWarnings("hiding") final Cluster cluster, final String keyspaceName) {

This comment has been minimized.

@amcp

amcp Mar 18, 2017

why is hiding the member cluster ok?

This comment has been minimized.

@pkendall64

pkendall64 Mar 19, 2017

Contributor

This is just our eclispe settings. I'll remove the annotation.

.flatMap(addTime -> Iterator.ofAll(keyMutations.getAdditions()).map(addition -> columnValueStore.insertColumn(key, addition, addTime)));
return Iterator.concat(deletions, additions)
.grouped(20) // XXX Pull this out as a configuration option

This comment has been minimized.

@amcp

amcp Mar 18, 2017

please pull out the batch size into configuration

import com.datastax.driver.core.ConsistencyLevel;
import com.google.common.base.Preconditions;
public class CQLTransaction extends AbstractStoreTransaction {

This comment has been minimized.

@amcp

amcp Mar 18, 2017

add classdoc

This comment has been minimized.

@samant-orionhealth

samant-orionhealth Mar 26, 2017

Contributor

Added classdoc.

@boliza

This comment has been minimized.

Contributor

boliza commented Mar 22, 2017

should the CQL storage Backend just a part of the module janusgraph-cassandra, not a single module?

@pkendall64 pkendall64 force-pushed the orionhealth:feature/cql-backend branch from 19854c0 to 20fa04c Mar 22, 2017

@janusgraph-bot

This comment has been minimized.

janusgraph-bot commented Mar 22, 2017

Please verify the committer name, email, and GitHub username association are all correct and match CLA records.

@janusgraph-bot janusgraph-bot added cla: no and removed cla: yes labels Mar 22, 2017

@pkendall64 pkendall64 force-pushed the orionhealth:feature/cql-backend branch 3 times, most recently from 9faef11 to 323f652 Mar 22, 2017

@janusgraph-bot janusgraph-bot removed the cla: no label Mar 22, 2017

@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented May 7, 2017

samant-orionhealth and others added some commits Mar 7, 2017

Initial implementation of CQL Cassandra backend using Datastax driver…
… and prepared statements

Signed-off-by: Samant Maharaj <samant.maharaj@orionhealth.com>
Signed-off-by: Paul Kendall <paul.kendall@orionhealth.com>
Signed-off-by: Jason Plurad <pluradj@us.ibm.com>
Update travis build, authors and contributors files
Signed-off-by: Paul Kendall <paul.kendall@orionhealth.com>

@pkendall64 pkendall64 force-pushed the orionhealth:feature/cql-backend branch from 90646ca to 37f2554 May 8, 2017

@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented May 8, 2017

I have squashed a lot of the commits so it shows the initial implementation included a lot of the fixes we applied along the way, and then the reviews and extra functionality. If there's any other squashing or rearranging, please let us know and we'll get them done.

@sjudeng

This comment has been minimized.

Contributor

sjudeng commented May 11, 2017

Can you squash 4c5672f, 3b00163, 7558097 and 2c11799 into a single commit, and 8415550, fc6fbf4 and 4850899 into a single commit? I think those go together well and the end result would be 7 commits, which seems reasonable to me.

pkendall64 and others added some commits Mar 20, 2017

Fixes as per reviews and add more unit tests, including tinkerpop tests
Signed-off-by: Paul Kendall <paul.kendall@orionhealth.com>
Signed-off-by: Samant Maharaj <samant.maharaj@orionhealth.com>
Modified usages of Future to use a managed ExecutorService so better …
…control the thread lifecycle

Signed-off-by: Samant Maharaj <samant.maharaj@orionhealth.com>
Implement missing methods to support ByteOrderedPartitioner and LOCAL…
… deployment

Various cleanup as per review comments

Signed-off-by: Paul Kendall <paul.kendall@orionhealth.com>
Add option to set cql protocol-version
Signed-off-by: Paul Kendall <paul.kendall@orionhealth.com>
Allow setting a system property for the cassandra to connect to for t…
…esting

Signed-off-by: Paul Kendall <paul.kendall@orionhealth.com>

@pkendall64 pkendall64 force-pushed the orionhealth:feature/cql-backend branch from 37f2554 to d0ea61e May 11, 2017

@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented May 11, 2017

Squashed commits as suggested

@sjudeng

This comment has been minimized.

Contributor

sjudeng commented May 11, 2017

@mbrukman Has all your feedback been resolved here?

@hsaputra,@fppt,@dylanht Are there any objections to merging this PR (pending thumbs up from @mbrukman)? I'm not a Cassandra user myself (@amcp not sure if you are) so current reviews might not have covered all aspects of the contribution. But that said there has been a lot of community interest on this and I think once merged these users will be able to try it out and provide additional feedback that can be tracked in subsequent issues/PRs if necessary.

@hsaputra

This comment has been minimized.

Member

hsaputra commented May 11, 2017

@sjudeng The code in general looks good, I didn't have check for correctness on runtime since I am not Cassandra user. But, one main comment is the dependency on io.javaslang which now changed to io.vavr. I am ok to change it later but better be the next PR submission bc there is a good reason why they change from javaslang to vavr =P

@sjudeng

This comment has been minimized.

Contributor

sjudeng commented May 11, 2017

@hsaputra Good call. Looks like the first version under io.vavr just came out yesterday so ... way to be on top of it!

@pkendall64, @samant-orionhealth I added #271 to track work to make this dependency update. Hopefully you'll be able to help out with this work after this PR is merged.

@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented May 11, 2017

@hsaputra

With tracked issue to update javaslang dependency, LGTM

<artifactId>guava</artifactId>
</dependency>
<dependency>
<groupId>io.javaslang</groupId>

This comment has been minimized.

@hsaputra

hsaputra May 11, 2017

Member

This one has been changed to vavr.io so not sure if you want to change it now given potential naming conflict from Oracle.

This comment has been minimized.

@pkendall64

pkendall64 May 16, 2017

Contributor

I've updated ticked #271 with a link to a branch in our repo with this migration. I'll create a PR once this is merged.

@fppt

fppt approved these changes May 12, 2017

@sjudeng

This comment has been minimized.

Contributor

sjudeng commented May 12, 2017

@pkendall64 Thanks for jumping on that. Do hold it for another PR though as I think we're about ready to go on this and I wouldn't want to hold it up to wait for another set of reviews on the new update.

@sjudeng

This comment has been minimized.

Contributor

sjudeng commented May 16, 2017

@mbrukman It looks like your feedback has been addressed here. Can you confirm?

@sjudeng

This comment has been minimized.

Contributor

sjudeng commented May 21, 2017

@pkendall64 A minor conflict in .travis.yml was created with merge of separate PR. I resolved it but it created another commit. I think it's fine but you're also welcome to rebase/squash to remove the merge commit if you'd like. Separately we'll need to look at getting the module covered better under Travis.

@mbrukman Unless you have any objections I'd like to move forward with the merge here. We have four approvals and I've confirmed that your feedback has been addressed.

@mbrukman

LGTM

@sjudeng sjudeng merged commit d139804 into JanusGraph:master May 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pkendall64

This comment has been minimized.

Contributor

pkendall64 commented May 21, 2017

@sjudeng I'm ok with the merge commit. Thanks for getting this merged and the thorough reviews from everyone involved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment