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

ReactiveHazelcastSessionRepository #2222

Open
wants to merge 2 commits into
base: 2.6.x
Choose a base branch
from

Conversation

DidierLoiseau
Copy link

@DidierLoiseau DidierLoiseau commented Jan 4, 2023

I was able to convert the Hazelcast4IndexedSessionRepository into a ReactiveSessionRepository using exclusively IMap’s *Async methods, with only small limitations, mostly the same as for Redis and Mongo:

  • most obvious one: there is no reactive equivalent to FindByIndexNameSessionRepository (see Add ReactiveFindByIndexNameSessionRepository #914), so those features cannot be supported (at least through an interface implementation)
  • FlushMode.IMMEDIATE cannot be supported since the Session interface is not reactive (I assume it is the reason why the Redis and Mongo implementations do not support it)
  • IMap.delete() does not have an async equivalent, so IMap.removeAsync() has to be used – see hazelcast#10702
    • possibly, the future returned by removeAsync() could be ignored?

I still don’t know why hazelcast/hazelcast#3622 was supposed to be needed for #831 so I would be glad to hear if this approach could cause any issue.

I think some refactoring should still be done to avoid code duplication, in particular on the event management, but possibly also in the usage of HazelcastSessionUpdateEntryProcessor.
The only code change I did in existing code is adding equals() and hashCode() (for Checkstyle) to HazelcastSessionUpdateEntryProcessor, as it makes it much easier to declare mocks for unit tests.

I am creating this PR against 2.6 because it is the version we are using, but it can as easily be merged in 2.7 since there are no differences in the Hazelcast 4 module between the two (just need to change the @since).
I didn’t check for 3.0 yet since I don’t have a real application to test it – I am aware merging this will require some manual operations due to the reorganization of the Hazelcast module but I hope this can be tackled later.

p.s.: I was a bit confused by the formatting rules. .editorconfig indicates LF, but gradle build seems to somehow expect CRLF.
Moreover it also uses latin1 for Java and XML files instead of UTF8, and continuation_indent_size does not seem to be a valid setting.

Closes #831

@DidierLoiseau
Copy link
Author

@marcusdacoregio I opened this PR more than a year ago. I didn’t do any follow-up as I was on a sabbatical, but now that I’m back… would it be possible to get it reviewed? I’m not working on the same project anymore but I don’t want to let this rot.

Note that there is also #1174 but maybe that one could be dropped since Hazelcast 3 support was removed in spring-session 3.

These are the only 2 old spring-session PR that are still open…

@marcusdacoregio
Copy link
Contributor

Absolutely @DidierLoiseau. I apologize for not getting to your PR earlier but we had some higher priorities tasks. I'm afraid that we won't be able to review it for 3.3, but I'll try to schedule it for 3.4.

@marcusdacoregio
Copy link
Contributor

It would be important if you could target the main branch and rebase your branch with main as well, fixing the conflicts and making the feature ready to be integrated.

@marcusdacoregio marcusdacoregio added type: enhancement A general enhancement in: hazelcast and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 19, 2024
@marcusdacoregio
Copy link
Contributor

You should also consider #1131 when revisiting your implementation

@DidierLoiseau
Copy link
Author

It would be important if you could target the main branch and rebase your branch with main

I’ll try to find some time to do that, but note that I won’t be able to test it since I don’t have an app with Spring Session 3.

You should also consider #1131 when revisiting your implementation

Actually, my implementation is compatible with the non-reactive implementation at the storage level (it’s “just” a conversion of the code), so it theoretically allows migrating an existing app to reactive without downtime, keeping the sessions active (maybe there are some limitations with the entry processor though, as suggested in #2463, so the old app should contain this implementation already).

I honestly wouldn’t want to dive into changing the storage mechanism, especially considering I won’t have a real app to test it. If you really want to have #1131 for the reactive implementation, then I don’t think I can do it.

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.

3 participants