fix: list found httpclient implementations and output selected one #4936
Conversation
metacosm
requested review from
manusa,
oscerd,
rohanKanojia,
shawkins and
sunix
as code owners
metacosm
force-pushed
the
client-selection
branch
from
0cc5ae2 to
2291af4
Compare
| return factory; | ||
| } | ||
|
|
||
| private static HttpClient.Factory getFactory(ServiceLoader<HttpClient.Factory> loader) { | ||
| HttpClient.Factory factory = null; | ||
| List<String> detectedFactories = new ArrayList<>(7); |
| @@ -73,7 +74,6 @@ public void before(BasicBuilder builder, HttpRequest request, RequestTags tags) | |||
| private static final Pattern IPV4_PATTERN = Pattern.compile( | |||
| "(http://|https://)?(?<ipAddressOrSubnet>(([01]?\\d\\d?|2[0-4]\\d|25[0-5])\\.){3}([01]?\\d\\d?|2[0-4]\\d|25[0-5])(\\/([1-2]\\d|3[0-2]|\\d))?)(\\D+|$)"); | |||
| private static final Pattern INVALID_HOST_PATTERN = Pattern.compile("[^\\da-zA-Z.\\-/:]+"); | |||
| private static final AtomicBoolean MULTIPLE_HTTP_CLIENT_WARNING_LOGGED = new AtomicBoolean(); | |||
There was a problem hiding this comment.
This could get a little chatty in some extreme case - the user has multiple http clients in the classpath, relies upon the default logic to create one of the client types, but specifies the other one programmatically.
There was a problem hiding this comment.
Shouldn't they all get picked up by the ServiceLoader at once, though?
There was a problem hiding this comment.
Or is it a matter that the method might be called several times, thus leading to the warning being output several times? If that's the case, this actually raises the question of whether or not we should record the chosen factory "once and for all" to avoid going through the resolution process every time a new client is created? Another potential issue is whether or not we want to support creating clients with implicitly different http clients?
There was a problem hiding this comment.
Or is it a matter that the method might be called several times, thus leading to the warning being output several times?
Yes, every client KubernetesClient creation that does not specify a HttpClient.Factory makes this call.
If that's the case, this actually raises the question of whether or not we should record the chosen factory "once and for all" to avoid going through the resolution process every time a new client is created?
Possibly. Some people would cache the ServiceLoader, its iterator will provide new instances should they be found later. However I would not want this to be static as the current HttpClientUtils is. There are several other usages of ServiceLoader in fabric8 that are created for each use. The only one that is not I believe is the KubernetesDeserializer because the resulting mapping is held statically.
Another potential issue is whether or not we want to support creating clients with implicitly different http clients?
That seems fine if your circumstances allow for that - you'd have to be doing something odd like dynamically installing new services or changing the priority of an existing one for the selection to change.
There was a problem hiding this comment.
I have to admit that's a little concerning because I was under the (maybe misguided) impression that creating a client was a rather fast operation. If we do http client resolution each time now, i.e. class path scanning, it might not be as much. If installing new services dynamically is not considered a core use case, it should be fine to always use the same resolved instance all the time (after the initial lookup).
There was a problem hiding this comment.
(maybe misguided) impression that creating a client was a rather fast operation
It should be especially in comparison to how long it should be used - any remote round trip will end up being orders of magnitude greater than the time to create the client.
If we do http client resolution each time now, i.e. class path scanning, it might not be as much
The ServiceToURLProvider has the same approach and you could make the same case about places that make repeated calls to loadClass.
If installing new services dynamically is not considered a core use case, it should be fine to always use the same resolved instance all the time (after the initial lookup)
It's mostly a question of where to put it - if you want to refactor so that it's held on the KuberenetesClientBuilder in some fashion that would be fine, but I would avoid doing anything statically.
There was a problem hiding this comment.
Taking a step back, this should be fine for almost all usage. There will not be repeated info / warn logs for our internal creation of httpclients (TokenRefreshInterceptor) because we pass the factory to it. Also a user wouldn't see additional logs if they keep re-using the same KuberenetesClientBuilder to build clients - as the factory is held on the builder.
metacosm
force-pushed
the
client-selection
branch
from
f87f571 to
eb26189
Compare
|
Kudos, SonarCloud Quality Gate passed! |
Description
Fixes #4935
Type of change
test, version modification, documentation, etc.)
Checklist