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

STAND-109 Replace the embedded mysql with mariadb4j #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

k4pran
Copy link

@k4pran k4pran commented Sep 17, 2024

See https://openmrs.atlassian.net/browse/STAND-109

Description

The change migrates the embedded database used by openmrs-standalone from mysql mxj to MariaDB4j (see https://github.com/MariaDB4j/MariaDB4j)

In order to keep the PR simple I opted to continue using the mysql driver instead of a MariaDB driver as mysql drivers are compatible with MariaDB. Using a MariaDB driver may be an improvement we can implement in a separate jira.

UPDATE

See comments on this PR for background, but this PR upgrades java version to 17. It also updates MariaDB4J to the version before the latest. I wasn't able to update on the latest version - MariaDB4j/MariaDB4j#926.

Due to the changes in version upgrades only 2.7.0-SNAPSHOT will now work

Testing

In addition to the new unit tests I also performed the below manual tests. I used the openmrs-standalone-x.x.x.zip that gets created for these tests, not sure if I need to do any validation on the other artifacts that get generated from packaging.

  • Ran versions 2.4.0, 2.5.0 and 2.6.0 in demonstration mode in windows and fedora
  • Ran versions 2.4.0, 2.5.0 and 2.6.0 in expert mode in windows and fedora in expert mode with custom setup using readme instructions
  • Ran versions 2.6.0 in demonstration mode in windows using commandline flag
  • Tested packaging and running in expert mode in both windows and linux (fedora)
  • Tested packaging in windows and running the resulting jar in linux (mostly worked but see below for one issue)
  • Tested changing the flag in properties 'reset_connection_password' to false so that the openmrs MariaDB user should keep the default password "test"

Issues and Limitations

Platform Differences

When I tried to package standalone in windows, it only worked in linux (fedora) using expert mode, when using demonstration mode it failed, it might be due to the database we create with the ibd files .I think this was an existing limitation rather than one brought in by my changes. The error I got is below

Installing MariaDB/MySQL system tables in '/home/k4pran/Downloads/stand-109-20240913T183639Z-001/stand-109/openmrs-standalone-2.5.0/database/data' ... 2024-09-16 17:02:13 139807890159424 [ERROR] InnoDB: MLOG_FILE_NAME incorrect:.\openmrs\users.ibd 2024-09-16 17:02:13 139807890159424 [ERROR] InnoDB: ############### CORRUPT LOG RECORD FOUND ################## len 100; hex

When I packaged the jar in linux it didn't have the above issue.

OpenMRS version 2.4

When trying with version 2.4.0 I was able to get the webapp working and logged on, but after logging on I get the following warning

**You cannot view or manage the Add Ons since you dont have the privileges and you are not an admin.

To get the permissions contact the system administrator.**

Additional Changes

Tomcat upgrade

I upgraded tomcat minor version to the latest 7.x as the current version filled the logs with the error

org.apache.tomcat.util.bcel.classfile.ClassFormatException: Invalid byte tag in constant pool: 19

Java upgrade

I upgraded java version to 8 17, initially because I had some issues with the static mocking with java 6. It allows us to use try-with-resources and a few other features while aligning with the java version of openmrs-core.

@k4pran k4pran marked this pull request as ready for review September 19, 2024 16:16
@dkayiwa
Copy link
Member

dkayiwa commented Sep 19, 2024

Where you able to successfully run this on Java 8? The last time i checked Mariadb4j, it required Java 17

@dkayiwa
Copy link
Member

dkayiwa commented Sep 19, 2024

Should i assume that this also works on mac os?

@k4pran
Copy link
Author

k4pran commented Sep 20, 2024

Where you able to successfully run this on Java 8? The last time i checked Mariadb4j, it required Java 17

Yes it runs on java8 but I couldn't use the most recent version of MariaDB4j for that reason

@k4pran
Copy link
Author

k4pran commented Sep 20, 2024

Should i assume that this also works on mac os?

I don't have a mac so wasn't able to test it on this. I tested on windows and fedora

@dkayiwa
Copy link
Member

dkayiwa commented Sep 22, 2024

Yes it runs on java8 but I couldn't use the most recent version of MariaDB4j for that reason

Would it be too much work to use the most recent version of MariaDB4j? Even if this means supporting only openmrs platform 2.7.0-SNAPSHOT which is the only version that supports Java 17?

@k4pran
Copy link
Author

k4pran commented Sep 26, 2024

Yes it runs on java8 but I couldn't use the most recent version of MariaDB4j for that reason

Would it be too much work to use the most recent version of MariaDB4j? Even if this means supporting only openmrs platform 2.7.0-SNAPSHOT which is the only version that supports Java 17?

Yea sure I can give that a go and will let you know how it goes

@dkayiwa
Copy link
Member

dkayiwa commented Sep 26, 2024

Yea sure I can give that a go and will let you know how it goes

That will be awesome! 👍

@k4pran
Copy link
Author

k4pran commented Oct 20, 2024

Yea sure I can give that a go and will let you know how it goes

That will be awesome! 👍

Updated to use java 17, also updated mariadb4j and mysql. I couldn't update mariadb4j to the very latest, but the one before the latest. I kept running into issues related to this on the latest version - MariaDB4j/MariaDB4j#926

@dkayiwa
Copy link
Member

dkayiwa commented Oct 20, 2024

Thank you so much @k4pran for this update! 👍

Off the top of your head, could you be knowing what is going on here? https://pastebin.com/sSjVtu6L

This is the environment that i am running: https://pastebin.com/fHPjMRgm

@k4pran
Copy link
Author

k4pran commented Oct 21, 2024

Thank you so much @k4pran for this update! 👍

Off the top of your head, could you be knowing what is going on here? https://pastebin.com/sSjVtu6L

This is the environment that i am running: https://pastebin.com/fHPjMRgm

I haven't seen the issue before myself, but it looks like mariadb4j has had some issues with macos. There are a few workarounds mentioned in this thread - MariaDB4j/MariaDB4j#411.

@dkayiwa
Copy link
Member

dkayiwa commented Oct 21, 2024

@k4pran let me try those workarounds and keep you updated with how it goes. Thanks for sharing that link. 👍

@k4pran
Copy link
Author

k4pran commented Oct 21, 2024

@k4pran let me try those workarounds and keep you updated with how it goes. Thanks for sharing that link. 👍

Sounds good, they are looking for help to work on this in the future so that is an option if we want to make changes in mariadb4j, I don't know much about it or if I could even get involved without a mac but I can certainly look into it so we can support macs in the future

@dkayiwa
Copy link
Member

dkayiwa commented Oct 21, 2024

A reasonably big number of our contributors are using macs. So this is the reason why it would be great to have this working on a mac. 😊

@dkayiwa
Copy link
Member

dkayiwa commented Oct 26, 2024

@k4pran i used my local mariadb binaries and then built the mariadb4j dependencies.

When i run the changes in this pull request with mvn clean and then mvn package -Dopenmrs.version=2.7.0-SNAPSHOT, i get a log that ends with this: https://pastebin.com/gizgN0Kt

Do you have any pointers off the top of your head?

@k4pran
Copy link
Author

k4pran commented Nov 3, 2024

@k4pran i used my local mariadb binaries and then built the mariadb4j dependencies.

When i run the changes in this pull request with mvn clean and then mvn package -Dopenmrs.version=2.7.0-SNAPSHOT, i get a log that ends with this: https://pastebin.com/gizgN0Kt

Do you have any pointers off the top of your head?

I was comparing your logs with mine, I think you are using a more recent MariaDB4j version

[INFO] artifact: file:/Users/danielkayiwa/.m2/repository/ch/vorburger/mariaDB4j/mariaDB4j/3.1.1-SNAPSHOT/mariaDB4j-3.1.1-SNAPSHOT.jar

Mine is

[INFO] artifact: file:/C:/Users/ryanm/.m2/repository/ch/vorburger/mariaDB4j/mariaDB4j/3.0.1/mariaDB4j-3.0.1.jar

If you see this comment I made earlier I couldn't update it to the very latest version (had to use the one before the latest)

Updated to use java 17, also updated mariadb4j and mysql. I couldn't update mariadb4j to the very latest, but the one before the latest. I kept running into issues related to this on the latest version - MariaDB4j/MariaDB4j#926

My guess is what is happening is related to the issue I linked or some other behaviour I didn't get to see on the latest version. Please try with version 3.0.1 and let me know if you still have issues. I will also leave my successful logs below if you need to use them as reference.

https://pastebin.com/qkSU1PPv

// Using the UID of the user that owns the data directory, we can execute this initial bootstrapping command:
// Note that on Windows MariaDB apparently does not implement this, still uses empty string password for the
// root user, so we can just use the root user.
if (rootPassword != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be when rootPassword is null?

Copy link
Member

Choose a reason for hiding this comment

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

@k4pran did you get a chance to look at the above comment? 😊

Copy link
Author

@k4pran k4pran Nov 28, 2024

Choose a reason for hiding this comment

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

You are right, but I realised this isn't needed for this version so I removed it


public static void stopMariaDB() throws ManagedProcessException {
if (mariaDB != null) {
mariaDB.stop();
Copy link
Member

Choose a reason for hiding this comment

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

Should we set mariaDB to null?

Copy link
Author

Choose a reason for hiding this comment

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

Yes good point, done

@dkayiwa
Copy link
Member

dkayiwa commented Nov 21, 2024

@k4pran here is an example of non technical people whose easiest entry is via the standalone version of OpenMRS: https://talk.openmrs.org/t/first-installation-error/44127. 😊

@dkayiwa
Copy link
Member

dkayiwa commented Nov 25, 2024

@k4pran when i run the output standalone on windows, i get this error on startup Data directory .....\database\data is not empty. Only new or empty existing directories are accepted for --datadir. Did you get the same when you run on windows?

@k4pran
Copy link
Author

k4pran commented Nov 26, 2024

@k4pran when i run the output standalone on windows, i get this error on startup Data directory .....\database\data is not empty. Only new or empty existing directories are accepted for --datadir. Did you get the same when you run on windows?

Did you change the MariaDB4J version or are you using a binary like you tried with mac? I only get that error on the most recent MariaDB4j version, that is the reason I am using the previous version. See here - MariaDB4j/MariaDB4j#926

@k4pran
Copy link
Author

k4pran commented Nov 26, 2024

@k4pran here is an example of non technical people whose easiest entry is via the standalone version of OpenMRS: https://talk.openmrs.org/t/first-installation-error/44127. 😊

Yea I can see how useful it can be, I will continue to try and make it easier to use even after this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants