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.

Sign up for GitHub

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

Jump to bottom

Fix performance issues with many offline sessions #15463

Merged
merged 1 commit into from Nov 15, 2022
Merged

Fix performance issues with many offline sessions #15463

merged 1 commit into from Nov 15, 2022

Conversation

hmlnarik
Copy link
Contributor

@hmlnarik hmlnarik added kind/bug Categorizes a PR related to a bug area/storage Indicates an issue that touches storage (change in data layout or data manipulation) labels Nov 11, 2022
@hmlnarik hmlnarik self-assigned this Nov 11, 2022
sguilhen
sguilhen reviewed Nov 11, 2022
View reviewed changes
Copy link
Contributor

@sguilhen sguilhen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @hmlnarik . Only one comment that doesn't really prevent merging this.

model/jpa/src/main/java/org/keycloak/models/jpa/session/JpaUserSessionPersisterProvider.java Outdated
* @param offlineStr
* @param useExact If {@code true}, then only client sessions from the user sessions
* obtained from the {@code query} are loaded. If {@code false}, then user session
* is taken as range, and all client sessions in between the minimum and maximum session
Copy link
Contributor

Choose a reason for hiding this comment

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

minor issue: the sentence doesn't look finished, seems like " are loaded" is missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, well spot. Fixed in the new commit

@hmlnarik hmlnarik force-pushed the 13340-perf-issues-with-many-offline-sessions branch from 0b7e317 to 11eb952 Compare November 11, 2022 12:54
@martin-kanis
Copy link
Contributor

Thanks for the PR. I have been thinking about this a bit and here are my thoughts:
Should we be worried about reintroducing IN-clause which was removed in last performance optimization attempt? If the userSessionsIds set used in IN-cause gets too big, performance will be probably bad. However, I think we should be OK because userSessionIds size should be reasonable. There are these use-cases:

  • load user session by user (not limited by first,max parameters as it is called with first=0, max=null, but limited by number of user session which user can has. Not sure if this can be a high number in real scenarios. Maybe yes, if someone creates many offline user sessions for a user by i.e password grant type)
  • load user sessions by client (this can be a high number but it is called only from services and endpoint is limiting max result size -- 100 by default)
  • load user sessions by interval - IN clause not used and method is called only from pre-loading

@hmlnarik What are your thoughts on this?

@hmlnarik
Copy link
Contributor Author

@hmlnarik What are your thoughts on this?

Thank you for the reply. These are exactly the arguments why I am in favour of introducing the IN clauses for the cases when the session is known in advance while keeping the interval for preloading, and this PR should reflect it.

mhajas
mhajas approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@hmlnarik hmlnarik merged commit 556146f into keycloak:main Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sguilhen sguilhen sguilhen approved these changes

@martin-kanis martin-kanis martin-kanis approved these changes

@mhajas mhajas mhajas approved these changes

Assignees

@hmlnarik hmlnarik

Labels
area/storage Indicates an issue that touches storage (change in data layout or data manipulation) kind/bug Categorizes a PR related to a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance Issues with many offline sessions
4 participants
@hmlnarik @martin-kanis @sguilhen @mhajas