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

Ensure the Quarkus HTTP Client Factory is always used by Kubernetes Client #32977

Closed
wants to merge 1 commit into from

Conversation

Sgitario
Copy link
Contributor

Fix #32937

@Sgitario Sgitario requested a review from geoand April 28, 2023 07:34
@Sgitario Sgitario changed the title Ensure the Quarkus HTTP Factory is always used by Kubernetes Client Ensure the Quarkus HTTP Client Factory is always used by Kubernetes Client Apr 28, 2023
@Sgitario
Copy link
Contributor Author

@geoand if the Quarkus HTTP Client Factory is the one that our Kubernetes Client instance must use, why not force the httpClientFactory to always use it?

Note that this will disable the ServiceLoader logic to find any other.

@geoand
Copy link
Contributor

geoand commented Apr 28, 2023

That's up for debate actually - cc @metacosm.

@geoand
Copy link
Contributor

geoand commented Apr 28, 2023

So let's wait and see what other think

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 28, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@metacosm
Copy link
Contributor

Which issue is this suppose to address? Regarding whether or not the factory should be used all the time, I don't know for sure. We still want people to be able to use a different http client implementation if they need to. That said, they could just provide their own Kubernetes client provider, configuring the client as they want so maybe that's fine.

@Sgitario
Copy link
Contributor Author

Which issue is this suppose to address? Regarding whether or not the factory should be used all the time, I don't know for sure. We still want people to be able to use a different http client implementation if they need to. That said, they could just provide their own Kubernetes client provider, configuring the client as they want so maybe that's fine.

#32937 is the issue we're trying to fix.
Basically, https://github.com/Sgitario/quarkus/blob/34264c28e3ac64dadd67cb5fbe3cb37feafe8d97/extensions/kubernetes-client/deployment/src/main/java/io/quarkus/kubernetes/client/deployment/KubernetesClientProcessor.java#L94 seems to not be working for DEV mode.

@geoand
Copy link
Contributor

geoand commented Apr 28, 2023

@geoand
Copy link
Contributor

geoand commented Apr 28, 2023

Actually, it probably does work as expected, but my guess is that the client that is created at build time is causing the issue.

In that case, we migjht want to limit disabling Service Discovery only to that client

@Sgitario
Copy link
Contributor Author

Actually, it probably does work as expected, but my guess is that the client that is created at build time is causing the issue.

Do you mean the one we created to deploy into OpenShift [1]? If so, I don't think you're right. We have disabled the Service Discovery for this client already.

@geoand
Copy link
Contributor

geoand commented Apr 28, 2023

Yeah I never tried it, just wondering.

@Sgitario
Copy link
Contributor Author

Basically, https://github.com/Sgitario/quarkus/blob/34264c28e3ac64dadd67cb5fbe3cb37feafe8d97/extensions/kubernetes-client/deployment/src/main/java/io/quarkus/kubernetes/client/deployment/KubernetesClientProcessor.java#L94 seems to not be working for DEV mode.

This is what we need to figure out before anything else

I would like to help with this. Though, if the Quarkus HTTP Client Factory is meant to always be used here, the simplest solution is this one.

Yet if you want me to investigate why RemovedResourceBuildItem does not work in DEV mode, I would need some pointers to know where to start looking into as I have no idea.

@geoand
Copy link
Contributor

geoand commented Apr 28, 2023

Yet if you want me to investigate why RemovedResourceBuildItem does not work in DEV mode, I would need some pointers to know where to start looking into as I have no idea.

I won't have time for this for the next 10 days or so (same goes for others who are familiar with this part of the code).
If you want to start looking, you should look into ClassTransformingBuildStep which uses the build item in question in order to build the transformed classes.

For a starting point for dev-mode, look into IsolatedDevModeMain#firstStart

@Sgitario
Copy link
Contributor Author

I've just confirmed that DEV mode is not consuming the RemovedResourceBuildItem build items at all.

Honestly, I don't see how DEV mode is meant to consume these build items, so I won't be helpful here. Anyway, I'm closing this pull request as the suggested solution seems not to be valid.

@Sgitario Sgitario closed this Apr 28, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Apr 28, 2023
@Sgitario Sgitario deleted the 32937 branch April 28, 2023 13:38
@metacosm
Copy link
Contributor

The warning will be improved in the next version of the fabric8 client and yes, it shows up because the Kubernetes client extension adds an additional factory, we should indeed try to mute the warning in the case where there's only the original vert.x factory and the one that the extension creates. I'll take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubernetes triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning: Multiple httpclient implementation in the classpath
3 participants