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

Add first-class Hazelcast support #544

Closed
wants to merge 1 commit into from

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Jun 9, 2016

Hazelcast support is currently based on generic MapSessionRepository which implements only SessionRepository interface. The goal of this PR is to provide first-class support based around dedicated session repository making Hazelcast up to par with other supported backends. This includes:

  • HazelcastSessionRepository which implements FindByIndexNameSessionRepository
  • spring-session-hazelcast module to improve user experience

Implementing HazelcastSessionRepository required upgrading Hazelcast to 3.6 since custom attributes extraction feature seemed the best way to implement FindByIndexNameSessionRepository#findByIndexNameAndIndexValue. Using this feature presents one challenge though - it requires configuring Hazelcast map (see HazelcastSessionRepository#configureSessionMap) which is not possible when using Hazelcast in client mode.

Still a TODO:

  • update documentation/javadocs
  • HazelcastSessionRepository unit tests
  • update existing integration tests

@rwinch Can you briefly review it and share your initial thoughts before I go further with this? @shakuzen I'd like you to get a quick look too.

/cc @stojsavljevic Having in mind #516, this might be of your interest also.

  • I have signed the CLA

@pivotal-issuemaster
Copy link

@vpavic Thank you for signing the Contributor License Agreement!

@rwinch
Copy link
Member

rwinch commented Jun 14, 2016

@vpavic Thanks for the PR! It appears the build is failing. Any chance you could take a look?

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label Jun 14, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Jun 14, 2016

Hey @rwinch, yes, it's failing as I didn't finish the configuration part that sets up the principal name extracting.

I just opened the PR to get your initial thoughts before finishing the config part and other remaining action points.

@rwinch
Copy link
Member

rwinch commented Jun 14, 2016

@vpavic Thanks for the fast response. I should have been more diligent about reading the message.

Is it possible to only write the session attributes that have changed vs saving the entire map every time? This has turned out to be a pretty important approach to reduce the possibility of race conditions.

@vpavic
Copy link
Contributor Author

vpavic commented Jun 14, 2016

@rwinch I've updated the PR.

The configuration part is done and the build now passes. I've also added Javadoc on new classes, other TODOs from the original comments are still pending but hopefully I'll get to finish those by the end of this week.

Regarding saving the session only when changes are detected, the new HazelcastSessionRepository uses plain MapSession so I'll just make use of the changes done there in #528.

Additionally, I've been in contact with @stojsavljevic over the past few weeks and he's started to do some work on #516 which is somewhat related to this.

@rwinch
Copy link
Member

rwinch commented Jun 14, 2016

@vpavic - These are great changes!

I don't know if it is possible, but I'd like to see an option similar to REDIS and JDBC repositories which only writes the session attribute that changed. For example, if I invoke:

session.setAttribute("foo","bar");
repo.save(session);
session.setAttribute("new","test");
repo.save(session);

The second save should not need to write the session attribute "foo". This helps reduce the likely hood of a race condition. At minimum it would be good to try to ensure if the last update time is changed, that only that needs to be written vs the entire Session.

This might not be possible with Hazelcast, but I'd like to make sure we investigate this possibility.

@vpavic
Copy link
Contributor Author

vpavic commented Jun 14, 2016

@rwinch Thanks for the feedback!

This might not be possible with Hazelcast, but I'd like to make sure we investigate this possibility.

I'm afraid it's not but I'll look into it over the next few days.

@vpavic
Copy link
Contributor Author

vpavic commented Jun 19, 2016

@rwinch I've updated PR with tests and have polished the general implementation a bit.

Regarding the individual attribute writes, both @stojsavljevic and I don't see how we could achieve that without dropping the traditional, single Map semantics on session persistence and employing a more complicated implementation that would dissect session persistence into multiple Maps.

@manderson23
Copy link
Contributor

Do we really want to drop support for Hazelcast in client mode? This was raised as an issue (#339) back when the original implementation was added so could be viewed as a regression.

@vpavic
Copy link
Contributor Author

vpavic commented Jun 20, 2016

@manderson23 The client mode support is not dropped with this change.

I assume my original comment confused you - sorry for that. It was a problem in the initial implementation that has been taken care of since, in the subsequent updates of this PR. I'll update to original comment to address this.

Also, you can see that the integration tests for client mode are passing now.

@manderson23
Copy link
Contributor

OK thanks.

FWIW I agree that individual attribute writes would be difficult with Hazelcast. I wouldn't like to see multiple Maps introduced.

@vpavic
Copy link
Contributor Author

vpavic commented Jun 23, 2016

@rwinch I've updated the PR with documentation and samples changes.

@vpavic vpavic mentioned this pull request Jun 23, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Jun 29, 2016

@rwinch Is it safe to assume this will make the 1.3.0.M1?

I've completed all the initially planned TODOs, regarding other changes, once this hits the master @stojsavljevic will follow up with a PR of his own to address the #516. There will also be some changes depending on the outcome of #557.

@rwinch rwinch removed the status: waiting-for-feedback We need additional information before we can continue label Sep 12, 2016
@rwinch rwinch self-assigned this Sep 12, 2016
@rwinch rwinch added this to the 1.3.0 M2 milestone Sep 12, 2016
@rwinch rwinch modified the milestones: 1.3.0 M2, 1.3.0 M1 Sep 12, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Sep 13, 2016

@rwinch I've updated the PR with some final polish, namely to use the current Hazelcast 3.6.5 release and to apply the changes equivalent to #608.

* Hazelcast.newHazelcastInstance(config);
* </pre>
*
* This implementation listen for events on the Hazelcast-backed SessionRepository and
Copy link
Member

Choose a reason for hiding this comment

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

listen → listens

@shakuzen
Copy link
Member

I left a few comments on JavaDocs. The actual code looks good to me. Great work, @vpavic

This commit improves existing Hazelcast support, which is based on `MapSessionRepository`, with dedicated `HazelcastSessionRepository` that implements `FindByIndexNameSessionRepository` contract.

Also a new `hazelcast-spring-session` module was added to provide dependency management for Hazelcast support.
@vpavic
Copy link
Contributor Author

vpavic commented Sep 13, 2016

@shakuzen Thanks for the review, I've addressed most of you comments with the latest PR update.

@rwinch
Copy link
Member

rwinch commented Sep 14, 2016

Thanks for the PR! This was merged via 3efb4c9c9e06dcd064ebe4a015f1e5492dd103fd

@rwinch rwinch closed this Sep 14, 2016
rwinch pushed a commit that referenced this pull request Sep 14, 2016
This commit improves existing Hazelcast support, which is based on
MapSessionRepository, with dedicated HazelcastSessionRepository
that implements the FindByIndexNameSessionRepository contract.

Also a new hazelcast-spring-session module was added to provide
dependency management for Hazelcast support.

Fixes gh-544
rwinch pushed a commit that referenced this pull request Sep 14, 2016
@vpavic vpavic deleted the hazelcast branch September 14, 2016 16:34
@rwinch rwinch modified the milestones: 1.3.0 M2, 1.3.0 M1 Sep 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants