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

FISH-1315 Improve loading time for REST application when there are many password aliases #5177 #5200

Merged
merged 4 commits into from
Apr 26, 2021

Conversation

AngelTG2
Copy link

ISSUE 5177 - Load of REST application is very slow (120 seconds) when there are many password aliases created

Description

This is a fix for the issue #5177
Load of REST application is very slow (120 seconds) when there are many password aliases created.

In this situation (800 password aliases), the cause of this slowness is that Microprofile OpenAPI reads the Microprofile Config when register an application in OpenAPI. PasswordAlias is a config source and it's necessary to read the keystore that is a taxing process if there are many password aliases.

One possible solution to resolve this issue is disable OpenAPI, but disabling OpenAPI seems that only affect to publish /openapi url. OpenAPI service is started even if OpenAPI is disabled and always register the application causing the slowness.

In this PR I've changed the registerApp method of OpenService class to prepare the document only if OpenAPI is enabled. With this modification, instead of taking 130 seconds to load an application it only takes less than 6.

Instead of modifying OpenAPIService class, the OpenApiApplicationContainer class could have been modified to avoid calling OpenApiService, but I thought it could have other side effects.

Important Info

Blockers

Testing

New tests

Testing Performed

Testing Environment

Documentation

Notes for Reviewers

@AlanRoth AlanRoth self-assigned this Apr 20, 2021
@AlanRoth AlanRoth added PR: CLA CLA submitted on PR by the contributor Type: Community Contribution labels Apr 20, 2021
@AlanRoth
Copy link

jenkins test please

@AlanRoth AlanRoth changed the title fixes #5177 FISH-1315 Improve loading time for REST application when there are many password aliases #5177 Apr 20, 2021
@smillidge
Copy link
Contributor

This is not really a comment on this PR as I understand what it is achieving but the fact that OpenAPI is loading all ConfigProperties at boot is a bug in itself as this could have a big performance impact on many ConfigSources and some people will want both OpenAPI and a slow config source or a source with many keys.

@AngelTG2
Copy link
Author

AngelTG2 commented Apr 20, 2021

@smillidge, I think you are right.

I was trying to find a solution to my problem, but probably some people need both OpenAPI and slow config sources.

It seems that OpenAPIConfiguration only needs several properties (mp.openapi.model.reader, mp.openapi.filter, mp.openapi.scan.lib, mp.openapi.scan.disable, mp.openapi.scan.packages, mp.openapi.scan.classes, mp.openapi.scan.exclude.packages, mp.openapi.scan.exclude.classes, mp.openapi.servers, mp.openapi.servers.path.* , mp.openapi.servers.operation.* and mp.openapi.schema.*)

I'm newbie in Microprofile and OpenAPI, so I ignore if these properties can be stored in a concrete config source or can be stored anywhere. If it can be stored anywhere, I can't think of any way to extract those properties without first reading all the config sources.

@smillidge
Copy link
Contributor

If OpenAPI is just looking for specific properties. That isn't of itself a bug, if it is loading all properties that would be. The Password Alias config source probably needs a review to ensure it is performant.

@AngelTG2
Copy link
Author

AngelTG2 commented Apr 20, 2021

I've reviewed the password alias config source:

    @Override
    public Set<String> getPropertyNames() {
        Set<String> propertyNames = new HashSet<>();
        if (store != null) {
            Iterator<String> keys = store.keys();
            while (keys.hasNext()){
                String key = keys.next();
                propertyNames.add(key);
            }
        }
        return propertyNames;
    }

@smillidge
Copy link
Contributor

@AngelTG2 thanks for the detailed analysis. This sounds similar to the problem described here #5187? Do you want to make a PR for the change you mention as it sounds reasonable?

@AlanRoth AlanRoth requested review from lprimak and OndroMih April 21, 2021 09:07
@AngelTG2
Copy link
Author

AngelTG2 commented Apr 21, 2021

@smillidge, I think the problem is similar to #5187
I've make a new commit (fixes #5177 v2).
I've reverted the changes in OpenApiService.java but I have corrected the indentation problems

@AlanRoth
Copy link

jenkins test please

@AlanRoth AlanRoth requested review from dmatej and AlanRoth April 22, 2021 09:16
@lprimak
Copy link
Contributor

lprimak commented Apr 23, 2021

@AngelTG2 I figured it would be easier to change myself than to explain :) Let me know if you are ok with this.

@lprimak
Copy link
Contributor

lprimak commented Apr 23, 2021

jenkins test

@AngelTG2
Copy link
Author

@lprimak. Perfect. I also think it will be faster that way. Thank you

@lprimak
Copy link
Contributor

lprimak commented Apr 23, 2021

Jenkins test

@AlanRoth AlanRoth merged commit 70a16f3 into payara:master Apr 26, 2021
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jun 9, 2021
FISH-1315 Improve loading time for REST application when there are many password aliases payara#5177
JamesHillyard pushed a commit to JamesHillyard/Payara that referenced this pull request Oct 28, 2021
FISH-1315 Improve loading time for REST application when there are many password aliases payara#5177
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants