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

ContentTypeFactoryImpl Getting Config values cause unneeded load #21619

Closed
wezell opened this issue Jan 27, 2022 · 5 comments · Fixed by #22209
Closed

ContentTypeFactoryImpl Getting Config values cause unneeded load #21619

wezell opened this issue Jan 27, 2022 · 5 comments · Fixed by #22209
Labels
Merged QA : Not Needed Release : 5.3.8.14 Included in LTS patch release 5.3.8.14 Release : 21.06.11 Included in LTS patch release 21.06.11 Release : 22.03.3 Included in LTS patch release 22.03.3 Release : 22.06 Team : Scout Type : Defect

Comments

@wezell
Copy link
Contributor

wezell commented Jan 27, 2022

In our containerized world, we do not need to support on the fly configuration changes or configuration reloading any longer. Instead of relying on apache classes, file watchers, and other blocking patterns, we should just read in the configuration at startup as an immutable map and use it as needed.

For example, in the ContentTypeFactoryImpl we call this whenever the class is inited (which happens a lot as it is not a singleton).

  final boolean LOAD_FROM_CACHE=Config.getBooleanProperty("LOAD_CONTENTTYPE_DETAILS_FROM_CACHE", true);

It turns out, our Config class is not really that performant. In this case, I see this a performance hotspot as java builds the exception's stack trace.

Screen Shot 2022-01-27 at 10 47 15 AM

There are multiple places in our code where you can see performance issues with our use of the class.
org.apache.commons.configuration.AbstractConfiguration

If we ever need a mechanism to update config on the fly in the future, we can write something then to do so by replacing the map, etc.

@wezell wezell changed the title Getting Config values cause unneeded load make Config immutable - getting Config values cause unneeded load Feb 2, 2022
@freddyucv
Copy link

Hey team! Please add your planning poker estimate with ZenHub @dsilvam @victoralfaro-dotcms

@jdotcms
Copy link
Contributor

jdotcms commented Feb 7, 2022

I was working on this PR: https://github.com/dotCMS/core/pull/19976/files
My idea was to create a service that provide the config on memory using an API instead of the Config
You can take a look

@freddyucv
Copy link

I was working on this PR: https://github.com/dotCMS/core/pull/19976/files My idea was to create a service that provide the config on memory using an API instead of the Config You can take a look

@jdotcms @wezell it still has work to do but I think it is going to solve the Bug, right?

wezell added a commit that referenced this issue May 3, 2022
@wezell wezell changed the title make Config immutable - getting Config values cause unneeded load ContentTypeFactoryImpl Getting Config values cause unneeded load May 3, 2022
@wezell
Copy link
Contributor Author

wezell commented May 3, 2022

PR #22143

@nollymar nollymar self-assigned this May 10, 2022
nollymar pushed a commit that referenced this issue May 10, 2022
@wezell wezell added the LTS : Next Ticket that will be added to LTS label May 12, 2022
fabrizzio-dotCMS added a commit that referenced this issue May 12, 2022
fabrizzio-dotCMS added a commit that referenced this issue May 13, 2022
@fabrizzio-dotCMS
Copy link
Contributor

new PR #22209

nollymar pushed a commit that referenced this issue May 13, 2022
* #21619 cherry picking changes

* #21619 adding logs for research

* #21619 fix on getBool

* #21619 fix on getBoolPropert test
@nollymar nollymar linked a pull request May 13, 2022 that will close this issue
@wezell wezell closed this as completed May 24, 2022
oidacra pushed a commit that referenced this issue Jun 13, 2022
* #21619 cherry picking changes

* #21619 adding logs for research

* #21619 fix on getBool

* #21619 fix on getBoolPropert test
@erickgonzalez erickgonzalez added the Release : 21.06.11 Included in LTS patch release 21.06.11 label Aug 9, 2022
erickgonzalez added a commit that referenced this issue Aug 9, 2022
@erickgonzalez erickgonzalez added the Release : 5.3.8.14 Included in LTS patch release 5.3.8.14 label Aug 31, 2022
erickgonzalez added a commit that referenced this issue Aug 31, 2022
@erickgonzalez erickgonzalez added Release : 22.03.3 Included in LTS patch release 22.03.3 and removed LTS : Next Ticket that will be added to LTS Severity : Engineering Requested labels Oct 24, 2022
erickgonzalez added a commit that referenced this issue Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged QA : Not Needed Release : 5.3.8.14 Included in LTS patch release 5.3.8.14 Release : 21.06.11 Included in LTS patch release 21.06.11 Release : 22.03.3 Included in LTS patch release 22.03.3 Release : 22.06 Team : Scout Type : Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants