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

replace the driver as it is not maintained anymore #125

Closed
oshai opened this issue Oct 2, 2018 · 36 comments
Closed

replace the driver as it is not maintained anymore #125

oshai opened this issue Oct 2, 2018 · 36 comments

Comments

@oshai
Copy link

oshai commented Oct 2, 2018

mauricio driver is not supported anymore. We created a clone of it in kotlin: https://github.com/jasync-sql/jasync-sql. will you consider using it instead? I can create a PR if you would like.

@vietj
Copy link
Contributor

vietj commented Oct 2, 2018

we will rather adop the reactive postgres client in vertx 4.0

unfortunately it does not support mysql, perhaps you can rather contribute there ?

@vietj
Copy link
Contributor

vietj commented Oct 2, 2018

for the record : https://reactiverse.io/reactive-pg-client/

@oshai
Copy link
Author

oshai commented Oct 2, 2018

Yes I can contribute the mysql part as well. How should I do that?

@vietj
Copy link
Contributor

vietj commented Oct 2, 2018

I think forking the reactive-pg-client and replacing the internal postgres parts by mysql would be the best, at least for the basic interactions (connection , query)

After this step we will have a pretty good idea of what is different and we can refactor the pg client to be not couppled to PG and use either postgres or mysql protocol.

@vietj
Copy link
Contributor

vietj commented Oct 2, 2018

if you are going this way, let me know and I'll subscribe to your fork for following and discussing.

@oshai
Copy link
Author

oshai commented Oct 2, 2018

I don't think that the suggestion is practical. It also couples the driver to vertex if I understand correctly. What about forking this repo and adapting? What is the downside?

@vietj
Copy link
Contributor

vietj commented Oct 2, 2018

why isn't it practical ?

@oshai
Copy link
Author

oshai commented Oct 2, 2018

There are a lot of features that are missing/different: notifications, cursor, streams, rx support etc'.

@vietj
Copy link
Contributor

vietj commented Oct 2, 2018

ah ok... I mean the idea is to not implement it completely but just start implementing basic (so other will throw UnsupportedOperationException).

The idea is that once basic interactions are implemented then I can use this to refactor the PG client and start introduce an SPI for PG and MySQL, so the core of the client can be used for both.

The current simplified architecture looks like:

PG API -> PG Connection management -> PG Commands

And I would like to go in the MySQL fork:

PG API -> MySQL Connection management -> MySQL Commands

Which would lead to the refactoring:

PG API -> Connection management -> (PG|MySQL) Commands

And then we figure out something such as the PG specific things are decoupled from the PG API.

I think the most important part in there is the middle layer Connection management that I would like to remain the same for all databases.

The API is just a front end to trigger commands at the end of the day.

I hope this makes sense.

@oshai
Copy link
Author

oshai commented Oct 3, 2018

@vietj - here is an example of how to convert this lib to jasync-sql: #126
if you prefer I can try to go the way you suggested, it might require more work.

@vietj
Copy link
Contributor

vietj commented Oct 3, 2018

@oshai on the middle term we want to support support the reactive-pg-client as it provides also a better API for interacting with database than the SQLConnection. We decided also to deprecate this project in 4.0 . So your contribution would have a greater impact on the project.

@oshai
Copy link
Author

oshai commented Oct 3, 2018

I forked the repo: https://github.com/oshai/reactive-pg-client
and created an initial implementation commit: oshai/reactive-pg-client@1f84bb0

Let me know what do you think.

@andy-yx-chen
Copy link
Contributor

@oshai thanks for your hard work on jasync-sql, as far as I know jasync-sql is more up-to-dated than the current dependency (Scala one), right? I don't understand why we should not just use jasync-sql instead of current dependency, if this plugin will not take the suggestion, I think we can just for this, port to jasync-sql (I already done), and publish to maven center, waiting for reactive-pg-client is not a good idea to go, as the current dependency is even more sophisticated. In open source world, people will always choose what is better. The only thing I want to point out is keep all hard working and don't let it die again, let's try to see if we can make more developers happier, here is my branch with jasync-sql https://github.com/andy-yx-chen/vertx-mysql-postgresql-client I also have a PR for this repo, but according to this thread, I don't think they are going to accept it, but anyways, I will use jasync-sql in my production, at least it has better support

@vietj
Copy link
Contributor

vietj commented Dec 10, 2018

@andy-yx-chen I think we can consider accepting your PR if you can support it on the mid/long term

@andy-yx-chen
Copy link
Contributor

@andy-yx-chen I think we can consider accepting your PR if you can support it on the mid/long term

Yes, I will keep contributing to both projects, jasync and this project, as we are high rely on reactive frameworks like vertx

@codepitbull
Copy link

On first sight I see a ton of good things in this idea. We would get rid of a Scala-dependency (makes my life easier) and we would get a maintained driver.
I am just hesitent about the impact this could have to users of the exisiting driver.
This is a rather big change and will impact allocation behavior and other areas.
I am not trying to qualify these changes, they can be both good and bad but they will be unexpected.
So if we want to do this we have to think about how we are goin g to handle it, especially for the future.

First: I talked with Maurico about a fork and he supports this idea. This would help us support this driver until we reache Vert.x 4 for which we planned to actually deprecate it.

Second: This PR and future maintenance is a rather big change and will cause quite some work for @andy-yx-chen .

If we want to switch out the underlying driver then I would do this with a Vert.x 3.7-Release and also reconsider our plan to deprecate the driver and instead keep it as a secondary option.

@andy-yx-chen
Copy link
Contributor

andy-yx-chen commented Dec 12, 2018

This would help us support this driver until we reache Vert.x 4 for which we planned to actually deprecate it.

That would be great, the motivation for me to drive this is because i already see many open issues regarding to current driver, as a open source community, we should move fast, that's why developers can trust. If the current driver is lacking of support, then we should think about what is the next, either pure java driver (should be at least as stable as current one) or something that is currently widely used.
To be honest, I am pretty happy to see @oshai 's work, though it's kotlin, but with support. I already use jasync in our production system because of trust.
I am fine with moving this forward with any agreed directions, but again, if we cannot fix some of the pending issues, then we should think about what should be the next

@andy-yx-chen
Copy link
Contributor

@oshai @vietj @codepitbull F.Y.I. though requesting for merge into 3.6 is not a good option, our team keep working on it, now we have implement the connectionTimeout and testTimeout which was removed in previous iteration (thanks @oshai to publish 0.8.55 for a improvement submitted by @bytewayio). I will keep working on it, as we may still need to add another connectionTimeout guard in AsyncConnectionPool, as it will put the request into waiting for more connections or waiting for test query

@vietj
Copy link
Contributor

vietj commented Dec 12, 2018 via email

@codepitbull
Copy link

I finally got some time to take a look at the PR.
It really looks good and I couldn't find any issues with the implementation.

This still leaves some organisational issues.

I'd suggest to move the Scala-based version into a separate branch and keep it as a backup.
We can't be sure that people didn't fiddle with some internals and I don't want to cause too much hamr to them, especially since we are so close to the end of the driver.

I also don't want to release this as a patch update.
We won't have API changes but replacing the whole groundworks of the driver is somethign I don't want to hide in a patch.

@vietj already created a 3.6 branch which should continue to contain the Scala driver.
I'd suggest to open a 3.7 branch and merge the changes there.

Would that work for everyone?

@andy-yx-chen
Copy link
Contributor

sounds good to me, thanks for reviewing!

@vietj
Copy link
Contributor

vietj commented Dec 18, 2018

@codepitbull could we instead live with the two codebases in the same project and support both in 3.6.x ? that would allow anyone to use the new impl and keep the old one.

if we ever do a 3.7, then we would remove the scala one.

@codepitbull
Copy link

@vietj how would we differentiate the two? Something like vertx-mysql-postgresql-client and vertx-mysql-postgresql-client-jasync-sql?

@vietj
Copy link
Contributor

vietj commented Dec 18, 2018 via email

@codepitbull
Copy link

@vietj I'd like to avoid having the Scala deps dragged into the Kotlin-version and vice versa.

@vietj
Copy link
Contributor

vietj commented Dec 18, 2018 via email

@codepitbull
Copy link

Exactly, the only clean way I see is to provide them as separate dependencies.
Anthing else will make things very confusing for users.

@vietj
Copy link
Contributor

vietj commented Dec 18, 2018 via email

@andy-yx-chen
Copy link
Contributor

So, what is the decision then? I am planning to enable MySQL 8.0 support as well as batching and fixing Json data issue

@vietj
Copy link
Contributor

vietj commented Dec 20, 2018

I think what @codepitbull said is the answer

@codepitbull
Copy link

I'll be working on this as soon as I get time (hopefully this weekend).
My plan is to add two submodules to this project:

vertx-mysql-postgresql-client
vertx-mysql-postgresql-client-jasync-sql

I'll keep the name of the original module so we don't confuse people.
I will sprinkle the original module with deprecation warnings so users know what to do.

@codepitbull
Copy link

I am back from family vacation and working on the changes :)
Might be a little bumpy until I am done ...

@oshai
Copy link
Author

oshai commented Jan 2, 2019

@codepitbull I suggest to use version 0.8.58 as it has some fixes. you can see details here: https://github.com/jasync-sql/jasync-sql/blob/master/CHANGELOG.md

@codepitbull
Copy link

PR is currently building, already updated to 0.8.58.
Version 0.8.59, which is mentioned in the changelog, is nowhere to be found.

@oshai
Copy link
Author

oshai commented Jan 2, 2019 via email

@oshai oshai closed this as completed Feb 8, 2019
@oshai
Copy link
Author

oshai commented Feb 8, 2019

Thanks for adding it.

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

No branches or pull requests

4 participants