Allow to set the executor used by OkHttp Dispatcher (for Virtual Threads support)

[victornoel]

Is your enhancement related to a problem? Please describe

I want to be able to use kubernetes-client with Java 21 Virtual Threads. It is already possible to override the executor used by the library itself using new KubernetesClientBuilder().withTaskExecutor(Executors.newVirtualThreadPerTaskExecutor()).build() but it's not possible to easily pass that same option to the OkHttp client used by the KubernetesClient.

Describe the solution you'd like

Either OkHttpClientFactory should reuse the same executor from the KubernetesClientBuilder or maybe better the KubernetesClientBuilder should have an extra .withHttpClientExecutor or the withHttpClientBuilderConsumer could allow to set it or something like that…

Describe alternatives you've considered

I have considered extending OkHttpClientFactory to override initDispatcher() and use new KubernetesClientBuilder().withHttpClientFactory(new VirtualThreadsOkHttpClientFactory()). The problem is that it requires copy-pasting its content since Dispatcher requires the new instance to be created with the Executor as a constructor parameter only.

Additional context

Not sure if there are other things that kubernetes-client needs to be passed an executor to?

[shawkins]

@victornoel I think we didn't commit to adding something at the KuberentesClientBuilder level to be more neutral with respect to the various http client implementation. In particular I don't think vertx exposes any configabilty wrt an executor.

Also the user has to be pretty sure about this scenario - over usage / misconfiguration of the HttpClient executor will quickly cause the KubernetesClient to fail in non-obvious ways, so it seemed fine to leave this as an advanced wiring exercise.

OkHttpClientFactory should reuse the same executor from the KubernetesClientBuilder

This unfortunately is messy with the way things are currently structured. The httpclient is created first, then the KubernetesClient is built on top of that. So it may be a little easier to think of this as an option for the KubernetesClient to use the executor from the HttpClient - if one is provided.

Trying to push down the executor from the KubernetesClient is possible, but would probably quite even more changes.

The problem is that it requires copy-pasting its content since Dispatcher requires the new instance to be created with the Executor as a constructor parameter only.

Spliting what's there into an init method and a configure method would address that.

cc @manusa

[manusa]

over usage / misconfiguration of the HttpClient executor will quickly cause the KubernetesClient to fail in non-obvious ways,

^^^ this

That executor has a very particular usage and is used under our very controlled circumstances.

So it may be a little easier to think of this as an option for the KubernetesClient to use the executor from the HttpClient - if one is provided.

I'd go this way around.

Spliting what's there into an init method and a configure method would address that.

I'm OK with splitting, or breaking down the initialization of each component in the OkHttp default factory.

...but it's not possible to easily pass that same option to the OkHttp client used by the KubernetesClient.

For version 7 we'll likely move away from OkHttp as the default HTTP client implementation.

Have you considered using any of the other HTTP client implementations (that prove to be more performant too)?

[victornoel]

Have you considered using any of the other HTTP client implementations (that prove to be more performant too)?

Ha, no, I didn't even think about it, good idea, is there one you would recommend? I'm not too bothered about performances (I want to use virtual threads because it allows to run my k8s controller with only one platform thread while it needs a bit of concurrency with IO blocking).

Spliting what's there into an init method and a configure method would address that.

Yep, I think it would be enough to answer my needs and most needs in general, especially in the light of what you said, which makes total sens to me:

over usage / misconfiguration of the HttpClient executor will quickly cause the KubernetesClient to fail in non-obvious ways, so it seemed fine to leave this as an advanced wiring exercise.

That executor has a very particular usage and is used under our very controlled circumstances.

[victornoel]

I've looked a bit at JdkHttpClientFactory and it's even harder to override because most of what you would need to fully implement what I need is not accessible:

• ShutdownableExecutor is not protected so I can't use it in my subclass
• jdkHttpClientImpl.getHttpClient() is not accessible so I can't correctly implement closeHttpClient()
• I don't want to use additionalConfig() because it means I will override the executor created by createNewHttpClientBuilder(), which will stay alive and running for nothing

The more I look at all this and the more I'm thinking I am not meant to override those classes :P Maybe the ideal solution would be for kubernetes-client to add a withVirtualThreads() option to the builder and then implement everything needed to use virtual threads everywhere? Like this users wouldn't have to try to make it work and potentially fail because we start touching things we shouldn't be?

[shawkins]

The more I look at all this and the more I'm thinking I am not meant to override those classes :P

No, you are - it's just that your are wanting to do something that wasn't previously accounted for. If that means making method changes to accomodate, that's ok.

ShutdownableExecutor is not protected so I can't use it in my subclass jdkHttpClientImpl.getHttpClient() is not accessible so I can't correctly implement closeHttpClient() I don't want to use additionalConfig() because it means I will override the executor created by createNewHttpClientBuilder(), which will stay alive and running for nothing

I'm not sure those are concens here? Aren't you providing the same executor to both the http client and the kubernetes client such that the closure at the kubernetes client level would handle this?

var es = Executors.newVirtualThreadPerTaskExecutor();
var supplier = new ExecutorSupplier() {

    @Override
    public Executor get() {
      return es;
    }

    @Override
    public void onClose(Executor executor) {
      es.shutdownNow();
    }

  };
var client = new KubernetesClientBuilder().withTaskExecutorSupplier(supplier).withHttpClientFactory(new JdkHttpClientFactory() {

  protected java.net.http.HttpClient.Builder createNewHttpClientBuilder() {
    return java.net.http.HttpClient.newBuilder().executor(es);
  }

}).build();

I know that isn't pretty, but something like that should work for you as a workaround/

Maybe the ideal solution would be for kubernetes-client to add a withVirtualThreads() option to the builder and then implement everything needed to use virtual threads everywhere?

This unfortunately isn't very easy given our target jre support. We either have to introduce reflective logic or different modules for the different jre support levels.

[victornoel]

I'm not sure those are concens here? Aren't you providing the same executor to both the http client and the kubernetes client such that the closure at the kubernetes client level would handle this?

Yep, you are correct, I ended up writing something very similar to your example there.

I think it's a good enough solution that I can have confidence in. Well, as long as the promise from all dependencies (jdk.httpclient and kubernetes-client) not to shutdown the executor apart from onClose() because the executor is shared by all httpclient created and some could be created and closed (e.g., in OpenIDConnectionUtils a client is created then closed). I think that is a safe assumption so I'm happy with this solution yes :) Thanks.

This unfortunately isn't very easy given our target jre support. We either have to introduce reflective logic or different modules for the different jre support levels.

Yep I see, and it's kind of consistent with how it's done even in the JDK (as jdk.httpclient shows us that they don't have some magic toggle to enable virtual threads).

So all in all, I guess that for okhttp (that I won't be using anymore :P), maybe separating instantiating the Dispatcher from configuring it should be enough, and for jdk.httpclient, I guess it's good enough as-is.

[manusa]

is there one you would recommend?

I haven't done any benchmarks, but I can tell you that both the Vert.x implementation and the Jetty implementation revealed issues with the client because they were doing things much faster :)

From a pragmatic perspective, Quarkus uses the Vert.x implementation. Also, for version 7.0, we'll likely default to Vert.x too (depending on what our JRE baseline is, if > 17, then it will be the JDK client instead).

[victornoel]

ok, thank you, I think I will stay with the JDK one as I don't have crazy performance needs and reducing the number of dependencies is always a good idea :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!