Skip to content
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

Initial support for cassandra backend #589

Closed
wants to merge 29 commits into from

Conversation

fitzoh
Copy link

@fitzoh fitzoh commented Aug 7, 2016

Provides a Cassandra Spring Session backend per #588, with dependencies provided via Spring Data Cassandra.

@fitzoh
Copy link
Author

fitzoh commented Aug 7, 2016

This is currently somewhat rough. A decent amount of cleanup and documentation is required, and it is light on unit tests.
Integration tests are copy pasted from the JDBC integration tests with some modifications, primarily around Expiration (JDBC expires sessions manually, while the Cassandra implementation makes use of TTL).

I had issues getting https://github.com/jsevellec/cassandra-unit and embedded Cassandra working, so the integration test currently depends on Cassandra running outside the process on localhost (I added Cassandra to the travis service list).

@@ -19,6 +19,8 @@ httpClientVersion=4.5.1
jedisVersion=2.8.1
h2Version=1.4.191
springDataMongoVersion=1.9.1.RELEASE
springDataCassandraVersion=1.5.0.M1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Ingalls milestone release really required here? Other Spring Data dependencies are on Hopper release train level ATM, so we should use 1.4.2.RELEASE.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lowered to the stable version, which also drops the Cassandra version from 3 to 2.

Tests fail on cass3, but pass on cass2 locally.

Pretty sure this means the travis tests will be broken until I can get embedded cass working.

@vpavic
Copy link
Contributor

vpavic commented Aug 9, 2016

I've added some comments on the code.
I guess the next step would be to add unit tests for the repository and CassandraHttpSessionConfiguration.

IMO it would be reeeally nice if you managed to get embedded Cassandra to work with integration tests.

insert.using(QueryBuilder.ttl(ttl));
}
catch (IllegalArgumentException e) {
log.info("Session has already expired, skipping save");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performing that check eagerly might help skip conversion (serialization)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, updated

@mp911de
Copy link
Member

mp911de commented Aug 9, 2016

Nice approach. I added my thoughts to the PR. Besides some basic configuration, you also might want to be able to configure the consistency level to tune throughput vs. consistency.

@fitzoh
Copy link
Author

fitzoh commented Aug 19, 2016

Finally got Embedded Cassandra working, not sure what I was missing initially

@vpavic
Copy link
Contributor

vpavic commented Dec 2, 2016

@jjzazuet We're currently in the RC phase of the 1.3.0 release which means this PR will be considered and more thoroughly reviewed for next releases (likely 1.4.0). Keep an eye on the assigned milestone, and the milestones page in general as an indication of release plans.

@jjzazuet
Copy link

jjzazuet commented Dec 2, 2016

@vpavic got it. Thanks!

@vpavic vpavic modified the milestone: 1.4.0 M1 Dec 16, 2016
@vpavic vpavic self-assigned this Dec 16, 2016
@vpavic vpavic added Data Store type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 13, 2017
@jjzazuet
Copy link

Hello everyone. Just wanted to ping to check if this PR is good for the 1.4.0 release. Thanks for your time!

@fitzoh
Copy link
Author

fitzoh commented Mar 28, 2017

At a minimum there are some conflicts that need to be updated. I'm a little short on time at the moment, but I think I just gave you push access to my repo if you want to clean those up @jjzazuet .

@jjzazuet
Copy link

@fitzoh yep, I can give those a shot. Thanks!

@vpavic
Copy link
Contributor

vpavic commented Mar 29, 2017

@jjzazuet @fitzoh You don't have to worry about conflicts, I've already taken care of that on my local branch for this PR when I started working on it. Thanks!

@jjzazuet
Copy link

jjzazuet commented Apr 1, 2017

@vpavic got it. Many thanks!

@vpavic
Copy link
Contributor

vpavic commented Apr 28, 2017

I'm afraid we are removing this from the 1.4.0.M1.

@rwinch and I have been discussing the supported data stores over the past few weeks, and the decision was made to limit the first-class supported data stores to Redis, JDBC and Hazelcast from 2.0.x on. See #768 for more details and background. As a part of this, existing GemFire and Mongo data stores have been removed in the master and will be deprecated in 1.4.0.M1.

@fitzoh @jjzazuet we'd like to encourage you to make this effort a Spring Session community extension project. We promote such extensions in Community Extensions section of the reference manual so please open a issue/PR to list your project in there once you set it up.

Thank you for your efforts. Also thanks to @mp911de for providing his feedback on this PR.

@vpavic vpavic closed this Apr 28, 2017
@vpavic vpavic removed this from the 1.4.0 M1 milestone Apr 28, 2017
@vpavic vpavic added status: declined A suggestion or change that we don't feel we should currently apply and removed Data Store type: enhancement A general enhancement status: on-hold labels Apr 28, 2017
@jjzazuet
Copy link

@vpavic @rwinch ay ay ay... alright... I mean, I still like the idea of keeping each data store implementation separate as per #768.

Anyway, it looks like milestone 2.0.0 M1 is nearing completion, so I guess we should target that Spring session version, right? @fitzoh , would you like me to start a spring-session-cassandra repository and prepare a Gradle build for pushing out artifacts into JCenter?

Thanks!

@vpavic
Copy link
Contributor

vpavic commented Jun 19, 2017

@jjzazuet @fitzoh release 2.0 is a good target - note the recently merged changes to Session API (#776) as this will have some impact on your implementation.

@cah-andrew-fitzgerald
Copy link

@jjzazuet just realized I never replied... Yeah, feel free to prep a repo/build if you'd like.

@vpavic
Copy link
Contributor

vpavic commented Jun 19, 2017

You can also use the newly created spring-session-data-mongodb and spring-session-data-geode repositories as references.

@jjzazuet
Copy link

@vpavic @cah-andrewfitzgerald alright. Yes it looks like both repositories are compiling against 2.0.0.BUILD-SNAPSHOT, so I'll give it a shot. Thanks!

@fitzoh
Copy link
Author

fitzoh commented Apr 4, 2018

Good news for anyone who might stumble upon this in the future:
@armeris has taken the code from the initial PR and created a standalone repo @ https://github.com/armeris/spring-session-cassandra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants