TRUNK-6418 Run liquibase checks and data imports only when version of core or modules changes #5603
Conversation
rkorytkowski
changed the title
|
@dkayiwa @IamMujuziMoses I'd appreciate yet another round of reviews, thanks! |
rkorytkowski
force-pushed
the
TRUNK-6418
branch
from
294a884 to
1cb8d71
Compare
| // GP may be null, but the session may contain an unflushed gp so | ||
| // we will do a final check with session.get below. It may happen, | ||
| // if flush is set to manual and running in a larger transaction. | ||
| return gp; |
There was a problem hiding this comment.
One test annotated with @StartModule was failing because of this. In general I think we have an issue with case-insensitive primary keys here. Session.get is using an L1 cache that is always case sensitive and we may run into strange errors, because of how we handle GPs. I will open an issue for that with a failing test case.
| if (useInMemoryDatabase()) { | ||
| runtimeProperties.setProperty(Environment.DIALECT, H2Dialect.class.getName()); | ||
| String url = "jdbc:h2:mem:openmrs;DB_CLOSE_DELAY=30;LOCK_TIMEOUT=10000"; | ||
| String url = "jdbc:h2:mem:openmrs;DB_CLOSE_DELAY=30;LOCK_TIMEOUT=10000;IGNORECASE=TRUE"; |
There was a problem hiding this comment.
Added to match what is in jupiter/BaseContextSensitiveTest
|
LGTM 👍🏾 |
| // 2.9.x should start at least 10% faster than 2.8.x | ||
| compareStartupPerformance("openmrs/openmrs-reference-application-3-backend:3.6.x-no-demo", | ||
| "openmrs/openmrs-reference-application-3-backend:3.6.x-no-demo", Duration.ofSeconds(0)); | ||
| "openmrs/openmrs-reference-application-3-backend:3.6.x-no-demo", -10); |
There was a problem hiding this comment.
Expected performance improvement to be 10% faster.
|
@rkorytkowski did you get a chance to run this with O3? |
|
@dkayiwa absolutely. Ran it with docker compose as well as it is running inside Startup performance test. |
|
@rkorytkowski i am assuming that you simply changed this https://github.com/openmrs/openmrs-distro-referenceapplication/blob/main/distro/pom.xml#L26 to 2.9.0-SNAPSHOT and then docker compose build, after which you did a docker compose run. Did you do anything else? |
|
No, it's not enough. Docker build doesn't have access to your local maven repo so it fetches 2.9.0-SNAPSHOT from OpenMRS maven repo. In order to test your locally build war you need to add to docker-compose.override.yml something like this: |
| /** | ||
| * Ask Liquibase if it needs to do any updates. | ||
| * Determine if Liquibase updates are required. If OpenMRS Core version did not change, then do not run any checks | ||
| * unless <b>force.setup</b> runtime property is set to <b>true</b>. |
There was a problem hiding this comment.
What do you think about renaming this setting to something else? When i look at force.setup i do not find it easy to relate it to Run liquibase checks and data imports only when version of core or modules changes 😊
There was a problem hiding this comment.
It is to run liquibase checks and data imports regardless if versions change or not. Any suggestions for naming?
There was a problem hiding this comment.
I will rename the property to optimized.startup and make it true by default. To force running liquibase you would have to set it to false. I will re-use the same property in TRUNK-6509.
|
Thanks @rkorytkowski for the clarification. 😊 Where should i expect the 10% improvement in startup time? For i have run O3 on core 2.8.2 and core 2.9.0 with your changes. They both take around 30 seconds to start. I am using docker for both. |
|
I have just remembered that i need to create a setting named |
|
Hi @dkayiwa, force.setup is only needed for development when you want to force running liquibase without version change. Do you see any improvements reported when running |
api/src/main/java/org/openmrs/api/impl/AdministrationServiceImpl.java
Outdated
Show resolved
Hide resolved
|
@rkorytkowski how long does |
|
Here is the full log: https://pastebin.com/uvG6mESY |
|
@dkayiwa see my results above... Your results are intriguing. Did you do/run anything else during execution? Even if I run a youtube video during execution it will slow down the build so you need to make sure your machine is focused on testing... |
rkorytkowski
force-pushed
the
TRUNK-6418
branch
from
1cb8d71 to
3a67d66
Compare
…f core or modules changes (cherry picked from commit 4723e71c3f39a6467ab6f4060dab42a694cd1a68)
rkorytkowski
force-pushed
the
TRUNK-6418
branch
from
3a67d66 to
9551c5c
Compare
|
@dkayiwa could we get this merged to see if it improves things when running performance tests with github actions? We can revert if it's not... |
|
No problem. Let me merge it right away as i continue testing it out. |
|
@rkorytkowski now i am getting this when i run |
|
Just in case you need it, here is the full log: https://pastebin.com/CRNcA8jj |
|
Interesting that i have just run it again and it passed with this: https://pastebin.com/kVM1FRNr |
|
Yes, I fixed it in 542ec4c I wonder why it's not improving on your machine. GHA consistently reports improvement as well as my machine. It is also more than 2x slower in your tests. It must be something specific to your setup... Let me create a branch of O3 with 2.9.x so you can run it outside of tests and see how it performs... |
|
@dkayiwa Could you please run "TAG=3.6.x-core-2.9 docker compose up" from openmrs-distro-referenceapplication? How does it perform compared to "TAG=3.6.x docker compose up"? |
|
Also please note that consecutive "TAG=3.6.x-core-2.9 docker compose up" startups are even faster thanks to using already expanded war (and soon jars). |
… core or modules changes (#5603) * TRUNK-6418: Run liquibase checks and data imports only when version of core or modules changes (cherry picked from commit 4723e71c3f39a6467ab6f4060dab42a694cd1a68) * TRUNK-6418: Follow up adjustments --------- Co-authored-by: IamMujuziMoses <[email protected]> (cherry picked from commit 55cf1c5)
… core or modules changes (openmrs#5603) * TRUNK-6418: Run liquibase checks and data imports only when version of core or modules changes (cherry picked from commit 4723e71c3f39a6467ab6f4060dab42a694cd1a68) * TRUNK-6418: Follow up adjustments --------- Co-authored-by: IamMujuziMoses <[email protected]> (cherry picked from commit 55cf1c5)
… core or modules changes (openmrs#5603) * TRUNK-6418: Run liquibase checks and data imports only when version of core or modules changes (cherry picked from commit 4723e71c3f39a6467ab6f4060dab42a694cd1a68) * TRUNK-6418: Follow up adjustments --------- Co-authored-by: IamMujuziMoses <[email protected]> (cherry picked from commit 55cf1c5)
4 participants
Description of what I changed
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6418
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 --amendI 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 --amendI ran
mvn clean packageright before creating this pull request andadded 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 masterThe reported startup performance improvements:
They will be even more significant for implementations having longer history of upgrades as we are only testing against a system upgraded only once from 2.8.x to 2.9.x.