spring-session icon indicating copy to clipboard operation
spring-session copied to clipboard

ReactiveHazelcastSessionRepository

Open DidierLoiseau opened this issue 3 years ago • 5 comments

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 #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 https://github.com/hazelcast/hazelcast/issues/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 avatar Jan 04 '23 16:01 DidierLoiseau

@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…

DidierLoiseau avatar Mar 19 '24 10:03 DidierLoiseau

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 avatar Mar 19 '24 12:03 marcusdacoregio

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 avatar Mar 19 '24 12:03 marcusdacoregio

You should also consider https://github.com/spring-projects/spring-session/issues/1131 when revisiting your implementation

marcusdacoregio avatar Mar 19 '24 12:03 marcusdacoregio

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.

DidierLoiseau avatar Mar 22 '24 00:03 DidierLoiseau