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

Enable state transfer even if a remote cache is enabled #592

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

ahus1
Copy link
Contributor

@ahus1 ahus1 commented Nov 17, 2023

This re-enables the state transfer even if the remote store is enabled.

There are some situations when listing user sessions for example in InfinispanUserSessionProvider where cache loaders are skipped, so all entries should be in the cache for this case. The state transfer ensures that this is the case when a node joins or leaves.

https://github.com/keycloak/keycloak/blob/c036980c3759b833d98030cf1299297a7ec97436/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java#L396-L397

@ahus1 ahus1 self-assigned this Nov 17, 2023
@ahus1
Copy link
Contributor Author

ahus1 commented Nov 17, 2023

@pruivo - if you have the time, please comment on this PR. I see this as a step backwards in terms of performance, but it seems to me that it is the only way to produce correct results.

@pruivo
Copy link
Contributor

pruivo commented Nov 17, 2023

@ahus1 the PR looks ok to me.

@ahus1 ahus1 marked this pull request as ready for review November 17, 2023 11:38
@ahus1 ahus1 merged commit f102db7 into keycloak:main Nov 17, 2023
2 of 3 checks passed
@pruivo
Copy link
Contributor

pruivo commented Nov 17, 2023

@ahus1 Having second thought, the code in [1] needs to be fixed. It won't behave properly in the following scenarios

  • if num-owners Keycloak servers crash, the sessions are lost from in-memory embedded Infinispan, and the iteration will miss those.
  • Keycloak receives an XML configuration file and users may configure it to limit the number of sessions in memory (eviction). Once again, the iteration won't be correct and won't return all sessions.

[1] https://github.com/keycloak/keycloak/blob/c036980c3759b833d98030cf1299297a7ec97436/model/infinispan/src/main/java/org/keycloak/models/sessions/infinispan/InfinispanUserSessionProvider.java#L396-L397

@ahus1
Copy link
Contributor Author

ahus1 commented Nov 17, 2023

I think all of that was done for performance reasons to not touch the remote store. I wonder how much performance impact we could expect here when we allow accessing the remote store. I need to understand better how Infinispan behaves.

@ahus1
Copy link
Contributor Author

ahus1 commented Nov 17, 2023

As discussed on the call: Using the remote store here would always fetch all entries from the remote store, which would have quite a performance penalty. Deferring optimizations to when we move to a remove Infinispan (possibly with Ickle queries)

@ahus1 ahus1 deleted the enable-state-transfer-for-remote-cache branch December 6, 2023 10:35
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