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

Trunk 6225: Excessive calls to DB on some endpoints when layout.name.template GP not set #4610

Closed
wants to merge 2 commits into from

Conversation

Lemmynjash
Copy link

Trunk 6225: Excessive calls to DB on some endpoints when layout.name.template GP not set

Description of what I changed

I cached the deserialized NameTemplate instance and deserialize it again when the global property value changes. This way, unnecessary SELECT statements for retrieving the global property value can be avoided for each queue entry retrieval. I overrode the globalPropertyChanged method to update the deserialized NameTemplate instance only when the layout.name.template global property changes. It updates the cached instance and the last known global property value accordingly.

Issue I am currently working on

see https://openmrs.atlassian.net/browse/TRUNK-6225

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

…lue has changed since the last time and if so update cachedNameTemplate
…lue has changed since the last time and if so update cachedNameTemplate
@Lemmynjash Lemmynjash marked this pull request as draft April 3, 2024 04:46
@gracepotma gracepotma requested a review from dkayiwa April 3, 2024 05:16
@chibongho
Copy link

@Lemmynjash looks reasonable, but I'll defer to @dkayiwa as I don't know this code base that well.

One of the maven tests failed with an NullPointerException. Can you fix it?

@dkayiwa
Copy link
Member

dkayiwa commented Apr 3, 2024

@Lemmynjash did you put this in draft because you are still finishing up some things before you are ready for review?

@Lemmynjash
Copy link
Author

Yes, I put this in draft because I am still working on finishing up the issue. I am following up on the reproduce provided by @chibongho and seeing if the solution provided fixes the issue mentioned.

@chibongho
Copy link

Hi @Lemmynjash, any update on this? Let me know if I can help.

@ibacher
Copy link
Member

ibacher commented Apr 18, 2024

One thing I'll note here is that the SELECT statement from the ticket is generated from the call Context.getAdministrationService().getGlobalProperty(OpenmrsConstants.GLOBAL_PROPERTY_LAYOUT_NAME_TEMPLATE), so fixing this issue will require not running that command at every instance. The API we have for this is a GlobalPropertyListener, and interface that can be registered and then will receive updates to any global properties it opts into. So a solution to this (in addition to caching the parsed XML in memory) would be to setup a kind of "holder" class that would load this property, register itself as a listener and then it only needs to change the value if the GP changes.

That's at least a vague outline of how to approach this.

@dkayiwa
Copy link
Member

dkayiwa commented May 10, 2024

Fixed at e878c63

@dkayiwa dkayiwa closed this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants