fix: Reuse KsqlClient instance for inter node requests #5624
Conversation
purplefox
changed the title
purplefox
changed the title
|
This should be tested with a long running pull query soak test which hammers at least two nodes as fast as possible with a high level of concurrency (thousands of concurrent requests over many connections), and where half the requests get forwarded to the other node. I believe @AlanConfluent and @vpapavas are working on this. I'd recommend that soak test is run before this is shipped. |
| final KsqlSecurityContextProvider ksqlSecurityContextProvider = | ||
| new DefaultKsqlSecurityContextProvider( | ||
| securityExtension, | ||
| RestServiceContextFactory::create, | ||
| RestServiceContextFactory::create, ksqlConfig, schemaRegistryClientFactory); | ||
| RestServiceContextFactory::create, ksqlConfig, schemaRegistryClientFactory, | ||
| sharedClient); |
There was a problem hiding this comment.
Does this mean that the shared client will be used for all internode communication? Also heartbeat and lag reporting?
|
Can we add a unit test to make sure we don't regress on this? |
| .setMetricsOptions(setUpHttpMetrics(ksqlConfig))); | ||
| vertx.exceptionHandler(t -> log.error("Unhandled exception in Vert.x", t)); | ||
|
|
||
| final KsqlClient sharedClient = new KsqlClient( |
There was a problem hiding this comment.
I have logic in DefaultKsqlClient that sets up the KsqlClient specifically for inter-node communication. Since it appears that the client module cannot depend on rest app configurations, I created a variant of the constructor for KsqlClient that takes a factory for HttpClientOptions so it can be setup in DefaultKsqlClient instead.
I think the main solution is to move out that logic to a central place so that it can be used by this or DefaultKsqlClient. Maybe an InterNodeKsqlClientFactory?
| @@ -554,7 +554,7 @@ public static KsqlRestApplication buildApplication(final KsqlRestConfig restConf | |||
| new KsqlSchemaRegistryClientFactory(ksqlConfig, Collections.emptyMap())::get; | |||
| final ServiceContext serviceContext = new LazyServiceContext(() -> | |||
| RestServiceContextFactory.create(ksqlConfig, Optional.empty(), | |||
| schemaRegistryClientFactory)); | |||
| schemaRegistryClientFactory, Optional.empty())); | |||
There was a problem hiding this comment.
This is already a singleton, so it's not a big savings, but you could use the same vertx object and KsqlClient here too, if you wanted. That way it wouldn't have to be optional and I think you might be able to remove some of the boolean sharedClients around.
|
Once this is fixed, we nede to merge this to 6.0.x / 0.10.x |
Feels like too big a change to put into a point release. Why not leave for 0.11? |
|
Closing as superseded by #5742 |
Description
Fixes: #5600
Previously a new KsqlClient instance was being created for each inter node request (e.g. pull query forwarding). There is a KsqlClient constructor which takes a sharedClient instance but this was never being used in the production code path.
This PR does the following:
Imho, the code around ServiceContext and KsqlRestApplication is very convoluted, has poor encapsulation and is very hard to reason about. I recommend this code is refactored and simplified, but that's a bigger task.
Testing done
Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Reviewer checklist