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

KEYCLOAK-18039: Optimise offline session load on startup #8012

Merged
merged 1 commit into from
May 13, 2021

Conversation

hmlnarik
Copy link
Contributor

@hmlnarik hmlnarik commented May 7, 2021

Updates and replaces #7902

@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 May 7, 2021
@hmlnarik hmlnarik added this to the 14.0.0 milestone May 7, 2021
@hmlnarik hmlnarik self-assigned this May 7, 2021
@hmlnarik hmlnarik requested a review from mposolda May 11, 2021 06:36
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@hmlnarik PR looks like great work, Thanks for it! I've added one comment inline. Did you tried some performance test? What DB you used? Do you have some numbers for the reference?

@@ -209,6 +217,28 @@ protected void initEmbedded() {
gcb.serialization().marshaller(new JBossUserMarshaller());

cacheManager = new DefaultCacheManager(gcb.build());
if (useKeycloakTimeService) {
replaceComponent(cacheManager, TimeService.class, new EmbeddedTimeService() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have KeycloakTestTimeService . How about moving it to model/infinispan to avoid using same code on multiple places?

Same probably applies to DefaultInfinispanConnectionProviderFactory.replaceComponent which is same code as InfinispanTestUtil.replaceComponent . This code uses infinispan internal stuff and it is a chance that during infinispan upgrade to newer version, there will be a need to change this code. So if we have same code with infinispan internals on two places in Keycloak codebase, it possibly increases the work for the infinispan upgrade in the future IMO.

Or just simplify the testsuite - maybe we can remove InfinispanTestUtil entirely and change TestingResourceProvider.setTestingInfinispanTimeService and TestingResourceProvider.revertTestingInfinispanTimeService to call DefaultInfinispanConnectionProviderFactory.setUseKeycloakTimeService or something like that? WDYT?

@hmlnarik
Copy link
Contributor Author

Model test failed is a known instability: https://issues.redhat.com/browse/KEYCLOAK-16999

Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@hmlnarik Thanks for the update. Maybe one last thing: It looks class KeycloakTestTimeService can be entirely removed now as it is not used anymore? It seems to be used only in the javadoc to TestingResource.setTestingInfinispanTimeService() , but maybe that javadoc can be slightly changed to not reference it anymore?

Co-authored-by: Hynek Mlnarik <hmlnarik@redhat.com>
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@hmlnarik Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants