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

[WIP] Switch to the mariadb connector from the mysql one. #4746

Merged
merged 2 commits into from
Aug 23, 2019

Conversation

kiwiroy
Copy link
Contributor

@kiwiroy kiwiroy commented Mar 10, 2019

Switch from com.mysql.cj.jdbc.Driver to org.mariadb.jdbc.Driver.

References

#4745

Checklist

Happy to work on these as required.

  • Change in CHANGELOG.md described
  • Tests created for changes (modified original)
  • Manually tested changed features in running JabRef (travis)
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes in principle but as I'm not to knowledgeable about this part of our ecosystem, I leave the decision to the other @JabRef/developers.

Please also add a changelog entry.

build.gradle Outdated
@@ -98,7 +98,7 @@ dependencies {
compile 'org.apache.pdfbox:pdfbox:2.0.14'
compile 'org.apache.pdfbox:fontbox:2.0.14'
compile 'org.apache.pdfbox:xmpbox:2.0.14'

compile group: 'org.mariadb.jdbc', name: 'mariadb-java-client', version: '2.4.0'
Copy link
Member

Choose a reason for hiding this comment

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

Please group this with the other db-related dependencies (mysql/postgre).
Can the mysql dependency be deleted now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 10, 2019
@kiwiroy kiwiroy force-pushed the mariadb-jdbc branch 2 times, most recently from 3c0fa0e to a0119a2 Compare March 10, 2019 23:00
@LinusDietz
Copy link
Member

Hey @kiwiroy, thank you for this contribution!
After some minutes of comparing the JDBC drivers, I'm not sure why mariadb is the obvious way to go. So please add more reasoning, both here in the comments, as well as a condensed version in an architecture decision record here https://github.com/JabRef/jabref/tree/master/docs/adr

This will help us to judge upon the benefits and consequences of this change.

@Siedlerchr
Copy link
Member

I'm curious: Would it be possible to have both connectors e.g. MYSQL and MARIADB together so that a user can choose between them or does this create conflics?

@kiwiroy
Copy link
Contributor Author

kiwiroy commented Mar 11, 2019

Just some comments here first:

I'm unsure if this is relevant, but this blog has some background information.
MariaDB provide a comprehensive feature comparison, though that is mainly
server features rather than client. As far as possible they maintain protocol
compatibility for clients.

For the linked issue (#4745) the MySQL connector does not connect to the PAM
enabled server despite claims. The MySQL connector documentation mention a
configuration property authenticationPlugins which is a "Comma-delimited list
of classes that implement com.mysql.cj.protocol.AuthenticationPlugin"
. A PAM
class, I'd expect dialog, cannot be seen in the connector either.

com.mysql.cj.protocol.a.authentication

The MariaDB implementations include a specific class for this.

This StackOverflow thread pointed me to the MariaDB connector
as a replacement that would achieve a connection and the comment in this one
suggests that performance will not be an issue.

The performance in mysql and with the mysql-connectorj was worse in all cases than mariadb

It is possible to have the two resident in the class path concurrently.

@kiwiroy
Copy link
Contributor Author

kiwiroy commented Mar 11, 2019

First pass at mysql and mariadb. Travis test suite passes and local testing of MariaDB connector against the database with PAM auth succeeds, while only changing the dropdown to MySQL results in the Unable to load authentication plugin 'dialog'. message.

N.B. As the driver is the only difference I chose to reuse the MySQLProcessor class master...kiwiroy:concurrently#diff-30b9c551de374dcefae08e56aad3dec9R577

@Siedlerchr
Copy link
Member

Yeah cool that it works! That would be best compromise here I think.

@kiwiroy
Copy link
Contributor Author

kiwiroy commented Mar 11, 2019

Right... Should this PR be closed and a new one created - after a little tidy up?

Also, any pointers on how this passed while including this line.

@Siedlerchr
Copy link
Member

Hm, that is really odd regarding the test. Try debugging the test locally or add a System.out.println before comparing the values to see the output

You can either reuse this PR if you want or use the other PR. Whatever you want

@Siedlerchr
Copy link
Member

@kiwiroy What is the current status here? It would be really nice to have this inlcuded

@kiwiroy
Copy link
Contributor Author

kiwiroy commented Apr 13, 2019

I’ve had a busy few weeks, will attend to it soon

@tobiasdiez tobiasdiez changed the title Switch to the mariadb connector from the mysql one. [WIP] Switch to the mariadb connector from the mysql one. Apr 24, 2019
@tobiasdiez tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 24, 2019
@tobiasdiez
Copy link
Member

@kiwiroy any update?

@LinusDietz
Copy link
Member

Closing due to inactivity. Feel free to reopen it if you have time to work on it.

@LinusDietz LinusDietz closed this Aug 23, 2019
@koppor koppor reopened this Aug 23, 2019
@koppor
Copy link
Member

koppor commented Aug 23, 2019

I resolved the conflicts and are going to merge it. I will add an ADR.

We keep only MariaDB. Reason: MySQL Connector/J 8.0 is licensed under GPL. Source: https://downloads.mysql.com/docs/licenses/connector-j-8.0-gpl-en.pdf

Without limiting the foregoing grant of rights under the GPLv2 and additional permission as to separately licensed software, this Connector is also subject to the Universal FOSS Exception, version 1.0, a copy of which is reproduced below and can also be found along with its FAQ at http://oss.oracle.com/licenses/universal-foss-exception

I understand that there is that FOSS exception, but I am not aware of the consequences - and that exception is not commonly known.

@koppor koppor merged commit 9aae35f into JabRef:master Aug 23, 2019
@koppor koppor mentioned this pull request Aug 23, 2019
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.

5 participants