Convert ReactiveConnection#close to a reactive method #725
Conversation
DavideD
added
design
|
@DavideD why's this still a draft? |
|
No reason, I just need to update the tests we merged yesterday and then I can switch it a regular PR |
|
@Sanne What do you think? In relation to Quarkus, is it safer to wait and merge it after the next release or we can merge it now? |
|
I think we need to sort these aspects quickly, so I'm inclined to suggest mering it now - but I wonder if t hat implies that we won't be able to upgrade Hibernate Reactive in Quarkus for a while as I understand it will require some changes there too? Did you have a look at how significant the changes would need to be on that side? As you know there's an upgrade of ORM which needs to get in Quarkus soon, which will break Hibernate Reactive so we'll need to be able to relase an HR build which keeps the Quarkus build afloat while this other work is settled. |
|
I think it's very important that we get breaking API changes released as soon as possible, to avoid creating more problems for users. If that includes fixes to the Quarkus extension and to Panache Reactive, then they also need to be prioritized. |
|
agreed @gavinking , but when we have both ORM core and HR bringing breaking changes to the table, we might need to address one at a time - which in this case would imply having an HR release which adapts to the changes in ORM but doesn't break other things. The alternative is to send both upgrades combined to Quarkus, which is entirely an option but I'm just warning that for it to be feasible we need to have an idea of the complexity: I can't delay the ORM upgrade in Quarkus further as there's many other works depending on it. |
|
Right, but I mean: it's pretty bad that we're not releasing connections! |
|
ah yes it's very bad. Don't get me wrong, I want both breaking changes integrated ASAP - just figuring out what's the fastest way to have it all at least resistance. |
* Replace deprecated method calls * Remove some Idea warnings * Replace with method reference * Change to static class a test entity
They made it reactive with the upgrade to Vert.x SQL client 4. We also have to update the tests becaus now they have to wait for the session to be close before moving to the next chained operation.
Now that the close method is reactive, we cannot make the session Autoclosable anymore.
|
Interestingly we can merge this as it doesn't need any changes to compile in Quarkus. Obviously we'll need to make changes to actually close the resources, but we don't need to wait with merging this particular patch. |
|
Merged! |
|
Damn! Wrong issue number in the commit messages. It's actually #716 |
Fixes #716
This should fix the issue about the close method not being reactive, I had to adapt our tests as well so there are a lot of changes to the test suite.
I'm not sure about the impact on Quarkus, considering that they are close to a release, it would be safer to merge this after their next release.