Shutdown ExecutorService(s) in multi node pipelines

[sazzad16]

Some discussions can be found in e193365

[sazzad16]

Thanks @jslopezgithub for reporting respective issue.

[yangbodong22011]

@sazzad16 The current PR still frequently creates and destroys threads. I strongly insist on making the thread pool static, let's keep working on #3333 .

[sazzad16]

The current PR still frequently creates and destroys threads. I strongly insist on making the thread pool static, let's keep working on #3333 .

@yangbodong22011 Yes. But it may take at least a couple of days. A quick fix for the issue is kind of necessary at this moment.

[dolavb]

We executed load testing with profiling and this change results in 45% of CPU being used for starting thread, incurring a latency cost equivalent to the network wait time for the Redis process to return on the completed command. Overall this change has been detrimental and would probably be better with executing the whole pipeline on a single thread in our case.

[sazzad16]

@dolavb I think you actually mean the changes in #3331. Right? If so, have you tried setting MULTI_NODE_PIPELINE_SYNC_WORKERS = 1?

[dolavb]

@dolavb I think you actually mean the changes in #3331. Right? If so, have you tried setting MULTI_NODE_PIPELINE_SYNC_WORKERS = 1?

@sazzad16 , sorry for the delay, I wanted to come with a Fix. Even with MULTI_NODE_PIPELINE_SYNC_WORKERS set to 1, this remains a poor experience event with 1:

• You are creating a thread that you do not need, as 1 would be equivalent as to executing with a single thread.
• That implementation is sub optimal as well as it can create threads that are not needed when the number of connection is lower than the value of MULTI_NODE_PIPELINE_SYNC_WORKERS.
• Further, this creates expensive resource as a side effect of the client call, which is overall a bad experience for the callers.

For my usage, this extra CPU will likely translate in large amount of money. I finally had to time to create a PR: 4070. I will move it out of draft in a bit, I want to rerun the profilers and get accurate number, that 40% is out of memory.

[szx1]

@sazzad16 I don't understand why a thread pool cannot use a static variable but instead is changed to a local variable with shutdownNow(). The latter has much lower performance compared to the former. What was the author's consideration?

[sazzad16]

@szx1 Consideration was performance in terms of data communication.

[dolavb]

@szx1 I agree that making this static is not the right approach, this forces the client to use that thread pool which might not be the right approach for that use case. Further, the exposition of a global variable through a static public field in the class to select the thread count of the pool has created a hard dependency on this approach, and broken encapsulation.

However, making the thread pool local is not the solution. As @sazzad16, the consideration here is the closing/syncing of multiple connections once the pipeline is closed/synced. The creation of a thread pool is CPU intensive, this is why we create thread Pool, and not thread Collections. We should not make this in a local variable that client keeps creating and shutting down for every pipeline. This might come with a benefit for people whit high communication cost, but for people who use pipelines and have a short communication time, that benefit is not there, and in fact it becomes detrimental.

I have created a PR 4070 to address this issue by allowing the clients to pass in an Executor service when creating a pipeline. I had to maintain the static one there, since this has now been leaked into a public interface. I believe the ideal situation following this PR would be to to add the ExecutorService as an optional field in the JedisClientConfig, and pass it down when needed. I am not sure if this need be done in the PR I have created.