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-102:Replace the embedded mysql with mariadb4j? #46

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sherrif10
Copy link
Member

@sherrif10 sherrif10 commented Jul 12, 2020

https://issues.openmrs.org/browse/STAND-102

cc @dkayiwa , i didnot entail to completely exclude mysql connectors since we cannot completely run on mariadb, we are to use mariadb in response to mysql, More suggestion will be appreciated thanks

@dkayiwa
Copy link
Member

dkayiwa commented Jul 12, 2020

@sherrif10 did you test this?

@sherrif10
Copy link
Member Author

Have been testing it and landed into some error, however am trying to get them fixed

@sherrif10
Copy link
Member Author

Hey @dkayiwa , the unfortunate thing is that i cannot connect to the database server using the mariadb, do i need to add mariadb connectors in place of mysql connectors, thats seems to be challenging me because am thinking of it at a perspective of other files that depend on mysql, And i think the main idea is not completely run on mariadb, Any suggestion thanks

pom.xml Outdated
@@ -1,4 +1,4 @@
<project xmlns="http//maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
<project xmlns="http//maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"

Choose a reason for hiding this comment

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

@sherrif10 thanks for working on this,was this change important?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thats a minor change

pom.xml Outdated
<groupId>mysql</groupId>
<artifactId>mysql-connector-mxj-db-files-fixed</artifactId>
<version>5.0.12-20170418</version>
<groupId>ch.vorburger.mariaDB4j</groupId>

Choose a reason for hiding this comment

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

@sherrif10 introducing maria db dependency only cannot make us run on maria db,we also need the maria db connector

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @HerbertYiga, however mariadb can work fine with mysql connectors without even any modification

Choose a reason for hiding this comment

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

@sherrif10 ohh yes,point noted.Could this be of use to you too https://mariadb.com/kb/en/mariadb-vs-mysql-compatibility/

Copy link
Member Author

Choose a reason for hiding this comment

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

@dkayiwa shared that to me earlier thanks again @HerbertYiga

@sherrif10
Copy link
Member Author

sherrif10 commented Jul 14, 2020

This work is under Progress and under testing. cc @dkayiwa @ibacher, Would you love reviewing this PR. thanks

@sherrif10
Copy link
Member Author

We can now run on mariaDB4J however testing it on standalone is still goin on

@sherrif10
Copy link
Member Author

Connections are now succesfull with latest changes as pers screenshot her
db. The link to openmrs talk thread here cc @dkayiwa @ibacher

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

These are just some minor fixes.

pom.xml Outdated
Comment on lines 86 to 87
<liquibasePluginArtifactId>liquibase-maven-plugin
</liquibasePluginArtifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<liquibasePluginArtifactId>liquibase-maven-plugin
</liquibasePluginArtifactId>
<liquibasePluginArtifactId>liquibase-maven-plugin</liquibasePluginArtifactId>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for coming up for this, Are you suggesting i remove this

Copy link
Member

Choose a reason for hiding this comment

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

No, just move it back to a single line like this: <liquibasePluginArtifactId>liquibase-maven-plugin</liquibasePluginArtifactId>

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhh thanks, let me do that

pom.xml Outdated
Comment on lines 89 to 96
<liquibaseDemoDataFileName>liquibase-demo-data.xml
</liquibaseDemoDataFileName>
<liquibasecielDataFileName>liquibase-ciel-data.xml
</liquibasecielDataFileName>
<liquibaseDemoDataFollowupFileName>liquibase-empty-changelog.xml
</liquibaseDemoDataFollowupFileName>
<liquibaseEmptyDbFollowupFileName>liquibase-empty-changelog.xml
</liquibaseEmptyDbFollowupFileName>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<liquibaseDemoDataFileName>liquibase-demo-data.xml
</liquibaseDemoDataFileName>
<liquibasecielDataFileName>liquibase-ciel-data.xml
</liquibasecielDataFileName>
<liquibaseDemoDataFollowupFileName>liquibase-empty-changelog.xml
</liquibaseDemoDataFollowupFileName>
<liquibaseEmptyDbFollowupFileName>liquibase-empty-changelog.xml
</liquibaseEmptyDbFollowupFileName>
<liquibaseDemoDataFileName>liquibase-demo-data.xml</liquibaseDemoDataFileName>
<liquibasecielDataFileName>liquibase-ciel-data.xml</liquibasecielDataFileName>
<liquibaseDemoDataFollowupFileName>liquibase-empty-changelog.xml</liquibaseDemoDataFollowupFileName>
<liquibaseEmptyDbFollowupFileName>liquibase-empty-changelog.xml</liquibaseEmptyDbFollowupFileName>

Copy link
Member Author

Choose a reason for hiding this comment

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

Finished

pom.xml Outdated
Comment on lines 108 to 109
<liquibasePluginArtifactId>liquibase-plugin
</liquibasePluginArtifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<liquibasePluginArtifactId>liquibase-plugin
</liquibasePluginArtifactId>
<liquibasePluginArtifactId>liquibase-plugin</liquibasePluginArtifactId>

Copy link
Member Author

Choose a reason for hiding this comment

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

Finished

pom.xml Outdated
Comment on lines 18 to 19
<ciel.dictionary.openmrs.version>1.9.9
</ciel.dictionary.openmrs.version>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<ciel.dictionary.openmrs.version>1.9.9
</ciel.dictionary.openmrs.version>
<ciel.dictionary.openmrs.version>1.9.9</ciel.dictionary.openmrs.version>

Copy link
Member Author

Choose a reason for hiding this comment

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

finished

pom.xml Outdated
Comment on lines 111 to 118
<liquibaseDemoDataFileName>liquibase-demo-data-18x.xml
</liquibaseDemoDataFileName>
<liquibasecielDataFileName>liquibase-ciel-data-18x.xml
</liquibasecielDataFileName>
<liquibaseDemoDataFollowupFileName>liquibase-rename-helper-tables-18x.xml
</liquibaseDemoDataFollowupFileName>
<liquibaseEmptyDbFollowupFileName>liquibase-rename-helper-tables-18x.xml
</liquibaseEmptyDbFollowupFileName>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<liquibaseDemoDataFileName>liquibase-demo-data-18x.xml
</liquibaseDemoDataFileName>
<liquibasecielDataFileName>liquibase-ciel-data-18x.xml
</liquibasecielDataFileName>
<liquibaseDemoDataFollowupFileName>liquibase-rename-helper-tables-18x.xml
</liquibaseDemoDataFollowupFileName>
<liquibaseEmptyDbFollowupFileName>liquibase-rename-helper-tables-18x.xml
</liquibaseEmptyDbFollowupFileName>
<liquibaseDemoDataFileName>liquibase-demo-data-18x.xml</liquibaseDemoDataFileName>
<liquibasecielDataFileName>liquibase-ciel-data-18x.xml</liquibasecielDataFileName>
<liquibaseDemoDataFollowupFileName>liquibase-rename-helper-tables-18x.xml</liquibaseDemoDataFollowupFileName>
<liquibaseEmptyDbFollowupFileName>liquibase-rename-helper-tables-18x.xml</liquibaseEmptyDbFollowupFileName>

Copy link
Member Author

Choose a reason for hiding this comment

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

Finished

pom.xml Outdated
Comment on lines 752 to 743
<url>http://mavenrepo.openmrs.org/nexus/content/repositories/public
</url>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<url>http://mavenrepo.openmrs.org/nexus/content/repositories/public
</url>
<url>http://mavenrepo.openmrs.org/nexus/content/repositories/public</url>

Comment on lines +1 to +2


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@@ -29,6 +32,7 @@ public static void main(String[] args) throws Exception {

for (int i = 0; i < args.length; i++) {
ServerLauncherSocketFactory.shutdown(new File(args[i]), null);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

}

}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}

@@ -0,0 +1,3 @@
CREATE USER 'openmrs'@'%' IDENTIFIED BY 'test';
GRANT ALL PRIVILEGES ON openmrs.* to 'openmrs'@'%';
FLUSH PRIVILEGES;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FLUSH PRIVILEGES;
FLUSH PRIVILEGES;

Copy link
Member Author

Choose a reason for hiding this comment

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

Finished, hope it looks fine now

@sherrif10 sherrif10 force-pushed the STAND-102 branch 4 times, most recently from dd8c7c0 to 99585aa Compare July 23, 2020 09:05
@sherrif10
Copy link
Member Author

Hello @dkayiwa @ibacher ,As we are trying to implement the class that launches standalone on mariaDB , included a simple class, could you have time and guide me on this, thanks

@@ -0,0 +1,35 @@
/**
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @ibacher @dkayiwa , back on this ticket, could you take a review about this class

@sherrif10
Copy link
Member Author

hey @ibacher @dkayiwa i consider first working on Trunk after we just depend on mariadb, i think this will lighten the workflow and dependencies, any suggestion

@achilep
Copy link

achilep commented Aug 11, 2020

thanks @sherrif10

Copy link

@deenoBaji deenoBaji left a comment

Choose a reason for hiding this comment

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

As linting mechanisms doesnot reccomend commented codes in production code. I reccomend removing the commented dependency and if need to consider that dependency in another scenario adding a complete documentation for that.

<scope>compile</scope>
<!--This can combine all the newest driver to access all databases -->

Choose a reason for hiding this comment

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

Please consider to remove this commented dependency.

@vorburger
Copy link

Hi folks, I'm the author and maintainer of MariaDB4j. OpenMRS is an awesome project and community! Just wanted to say Hi. Hope MariaDB4j works great for you? I've just released a new version a few days ago.

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