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

Don't use VertxRecorder to get Vertx instance #167

Closed
wants to merge 1 commit into from

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 14, 2023

No description provided.

@vsevel
Copy link
Contributor

vsevel commented Sep 14, 2023

can't validate the change, but we are going to fetch your branch and try on a project internally. thanks!

@vsevel vsevel requested a review from kdubb September 14, 2023 11:42
@geoand
Copy link
Contributor Author

geoand commented Sep 14, 2023

👌🏼

@kdubb
Copy link
Contributor

kdubb commented Sep 14, 2023

@geoand Please look at #109 as I referenced in the other thread. This is almost exactly how I originally wrote it and it was switched to using VertxRecorder in #109 (you opened the PR 😏)

@vmargerin
Copy link

@vsevel Changes validated on my side

@geoand
Copy link
Contributor Author

geoand commented Sep 14, 2023

Oh right... I have no memory of that whatsoever

@kdubb
Copy link
Contributor

kdubb commented Sep 14, 2023

Maybe something has changed in 3.4 that allows that to work again?

Here is your original problem statement for #78...

Essentially the problem is that when creating the synthetic bean for the PgPool, Quarkus loads the CredentialsProvider (via CDI) which then attempts (in this case) to load Vertx from CDI. This last load will fail because Vertx is also a synthetic bean.

@kdubb
Copy link
Contributor

kdubb commented Sep 14, 2023

I thought this use case had test coverage via quarkus. If it doesn't, it should be added to ensure this doesn't regress as we attempt to change this.

@geoand
Copy link
Contributor Author

geoand commented Sep 14, 2023

It probably works again by luck.

However I do know what is needed to fix it - I might have time to address the deep problem I described in #78 in Quarkus.

@geoand
Copy link
Contributor Author

geoand commented Sep 14, 2023

It turns out that the change is breaking

@geoand
Copy link
Contributor Author

geoand commented Sep 15, 2023

quarkusio/quarkus#35949 should fix things once and for all. With that change, one would be able to obtain Vert.x from CDI without any timing issues creeping in.

@vsevel
Copy link
Contributor

vsevel commented Sep 19, 2023

regardless of wether or not quarkusio/quarkus#35949 (comment) gets backported to 3.4, I think we need to go ahead with that fix. ok for you @kdubb ?

@geoand
Copy link
Contributor Author

geoand commented Sep 19, 2023

If worse comes to worse, you can always temporarily use VertxEventBusConsumerRecorder instead of VertxRecorder

@vsevel
Copy link
Contributor

vsevel commented Sep 19, 2023

you can always temporarily use VertxEventBusConsumerRecorder instead of VertxRecorder

@geoand so that would be an interim fix for 3.4, and we get your fix for 3.5?
it comes down to whether or not quarkusio/quarkus#35949 is going to be backported @gsmet @cescoffier ?

@geoand
Copy link
Contributor Author

geoand commented Sep 19, 2023

@vsevel correct on both counts

@kdubb
Copy link
Contributor

kdubb commented Sep 19, 2023

@vsevel Go ahead with which fix? This PR? I have trouble accepting a PR that fixes the 3.4 build but we know doesn't work with 3.4. This is near and dear to myself as we use this feature we would be breaking.

IIUC It sounds like we can change this PR to use VertxEventBusConsumerRecorder and then if/when #35949 is merged/backported we then just revert this PR and & #109.

@gsmet
Copy link
Member

gsmet commented Sep 19, 2023

Please use VertxEventBusConsumerRecorder for now so that we can get a release compatible with 3.4 before I release the Platform tomorrow around noon.

@vsevel
Copy link
Contributor

vsevel commented Sep 19, 2023

@gsmet I will work on this tomorrow morning

@vsevel
Copy link
Contributor

vsevel commented Sep 20, 2023

this PR will continue in the context of quarkus 3.5

@vsevel
Copy link
Contributor

vsevel commented Oct 11, 2023

@geoand is this the plan to upgrade to CR1 today and finalize this PR?
do you need some help?

@geoand
Copy link
Contributor Author

geoand commented Oct 11, 2023

Sure yeah, I can update the PR once CR1 is out

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.

6 participants