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 support to store sessions in Redis #310

Closed
wants to merge 3 commits into from

Conversation

msellinger
Copy link

No description provided.

if ( _log.isDebugEnabled() ) {
_log.debug( "Trying to store session in memcached: " + session.getId() );
}

try {
storeSessionInMemcached( session, data );
return new BackupResult( BackupResultStatus.SUCCESS, data, attributesData );
} catch (final ExecutionException e) {
Copy link
Owner

Choose a reason for hiding this comment

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

This one wrapped all exceptions that were thrown by the future task - without having further context I'd say that Exception or Throwable should now be caught as a proper replacement.

Markus Ellinger added 2 commits August 25, 2016 11:47
…ation using executor service in Redis client, some more comments and cleanup
if (exp == 0)
return true;
else
return jedis.expire(kb, convertExp(exp)) == 1;
Copy link
Owner

Choose a reason for hiding this comment

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

If the first attempt failed (where jedis.setnx actually caused the exception), wouldn't the next try (with another connection) potentially expire an existing session?

Copy link
Owner

Choose a reason for hiding this comment

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

Nevermind, jedis.setnx couldn't have caused the exception because _setCompleted wouldn't be true then. Instead jedis.expire must have caused the first exception. Therefore an existing session should not be expired by this call.

@magro
Copy link
Owner

magro commented Aug 28, 2016

@msellinger FYI: I'm starting to work on this, first things was a rebase/squash. The WIP is in branch pr-18-redis-support ... (if I don't forget it I'm marking resolved stuff with a 👍 )

@magro
Copy link
Owner

magro commented Aug 28, 2016

I'm through it, tests passed, pushed changes to pr-18-redis-support (I'll squash commits before merging...). Would be great if you could use this for testing.
Next step is the storageUrl thing, and to get rid of spymemcached dependency being required if redis is used.

@msellinger
Copy link
Author

Awesome work, thank you!! We are setting up our staging environment on AWS this week and I will use the version built from this branch for testing.

@magro
Copy link
Owner

magro commented Aug 29, 2016

👍

@magro
Copy link
Owner

magro commented Sep 8, 2016

Hi Markus,

I just merged the branch and released 2.0.0. I didn't manage to introduce "storageNodes" deprecating "memcachedNodes". Thanks a lot for this contribution again!

Cheers,
Martin

@magro magro closed this Sep 8, 2016
@msellinger
Copy link
Author

Thank you, that's exciting. What about adding documentation to use Redis? Do you need some input for that? I see that I seem to be able to "Edit" the Wiki (is it normal that this is world-writeable?), but should I? Also, maybe on the start page one should mention that as of 2.0.0, not only memcached but also Redis is supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants