-
Notifications
You must be signed in to change notification settings - Fork 7.4k
UserSession Offline removed from DB if not in cache #40831
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
Conversation
Thank you for the PR. I assume you want to enable users to have a size-limited offline user session and client session cache. If you want to go down that route, there are IMHO some additional changes necessary:
Note that when we upgrade the non-persistent user sessions like this, we would need to maintain this code side-by-side with the persistent user sessions. Some of the logic (for example batching the writes to the DB) are quite different in the two implementations, and will require understanding both in the team. |
What I want is Yes, I want the users to have options: small deployments to use unbounded caches and large deployment with bounded caches.
Do you want me to do them in this PR or another one?
Read-only is fine; the problem is when updates are involved. Because we use
Honestly, the only database accesses I see on Lines 175 to 191 in 4dc4de7
|
Due to the latest hardcoded safeguards here, I think we need to do additional changes, otherwise users won't be able to use it with a size-limited cache, as we would simply ignore those settings: Lines 220 to 233 in f4d5fa6
Please schedule a 1:1 if you want to discuss it in more details, that might be quicker. |
@ahus1 I updated/removed the safeguards. |
I had a look, updated some docs and pushed it in a more opinionated and IMHO safe-to-use direction. Please let me know your thoughts. Happy to jump on a call during the next days. |
@ahus1 I'm ok with the changes. |
Fixes keycloak#40754 Signed-off-by: Pedro Ruivo <pruivo@redhat.com>
Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it another test locally and approve it. Thank you for the PR.
@pruivo - it would be nice if you could add the enhancement of parallel loading of client sessions also to PersistentUserSessionProvider#importSessionsWithExpiration. Thanks! |
Fixes #40754
The method
org.keycloak.models.sessions.infinispan.InfinispanUserSessionProvider#importUserSession(org.keycloak.models.RealmModel, org.keycloak.models.UserSessionModel)
was rewritten. The main differences are that I'm usingputIfAbsent
to import the session, and I'm importing all the client sessions in parallel.I also took the opportunity to fix the code warnings (unused code, parameters with constant values, missing types, etc).