Introduce thread pool for multi node pipeline

[yangbodong22011]

This PR mainly includes the following changes：

1. Introduce JedisThreadFactoryBuilder and JedisThreadPoolBuilder to standardize the creation of thread pools (this is beneficial for future debugging).
2. make executorService as a static object, created by default (even if the user does not use MultiNodePipeline), this can be recycled by multiple pipelines instead of frequent resource creation and destruction.
3. Users can customize the executor through setExecutorService.
4. submit method catch reject exception and record.

[sazzad16]

make executorService as a static object, created by default (even if the user does not use MultiNodePipeline)

1. From my understanding, if MultiNodePipelineBase class isn't accessed, those thread pool classes won't be accessed and so initialized. Am I missing something?

2. WDYT about adding an option to disable this common thread pool? I.e. using system property?

[yangbodong22011]

• From my understanding, if MultiNodePipelineBase class isn't accessed, those thread pool classes won't be accessed and so initialized. Am I missing something?

Previously, every time a new MultiNodePipelineBase was created, an Executor would be created and destroyed immediately after use (very resource-intensive)

So I made it a static object, created by default, but if not used, no thread will be created (just a partial memory footprint)

• WDYT about adding an option to disable this common thread pool? I.e. using system property?

I don't tend to add such options. Another way is:

We only add the setExecutorService interface, and the executorService is null by default. When the user does not set it through setExecutorService, it will be executed serially, as before; when the user sets the executor, it will be executed through the executor.

[dengliming]

IMO, I suggest that it is better to supply this parameter directly to the constructor method rather than the setter method. : )

[sazzad16]

@dengliming This is set once, works every time approach :)

[yangbodong22011]

IMO, I suggest that it is better to supply this parameter directly to the constructor method rather than the setter method. : )

We need to pass the executor when new JedisCluster or new JedisSharding, and need a clear name to avoid misunderstanding by users, and there are many modifications(JedisCluster->ClusterPipeline->MultiNodePipelineBase).

But the benefit is that we can create the executor lazily, if the user passes it, we use it; otherwise, we can create a static object.

But I personally prefer the current code, and it is one-time and clear to set directly through the MultiNodePipelineBase#setExecutorService interface (About cost: there is only a part of memory usage, if the user is not using it, no thread will be created).

[sazzad16]

@yangbodong22011 yangbodong22011#1

[codecov-commenter]

Codecov Report

Patch coverage: 60.17% and project coverage change: -0.07% ⚠️

Comparison is base (71204c5) 71.15% compared to head (93e70bb) 71.08%.

❗ Current head 93e70bb differs from pull request most recent head 03778be. Consider uploading reports for the commit 03778be to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3333      +/-   ##
============================================
- Coverage     71.15%   71.08%   -0.07%     
- Complexity     4763     4774      +11     
============================================
  Files           278      280       +2     
  Lines         15014    15111      +97     
  Branches       1057     1065       +8     
============================================
+ Hits          10683    10742      +59     
- Misses         3864     3891      +27     
- Partials        467      478      +11     

Files Changed	Coverage Δ
...redis/clients/jedis/JedisThreadFactoryBuilder.java	53.33% <53.33%> (ø)
...va/redis/clients/jedis/JedisThreadPoolBuilder.java	61.53% <61.53%> (ø)
...ava/redis/clients/jedis/MultiNodePipelineBase.java	72.36% <68.96%> (-0.65%)	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sazzad16]

@yangbodong22011 yangbodong22011#2

[yangbodong22011]

@yangbodong22011 yangbodong22011#2

@sazzad16 commented.

[asolimando]

@yangbodong22011 @sazzad16, we have a down-stream fork of Jedis exactly for the missing capability of re-using the thread pool.

The PR seems in good shape and there are no pending comments from what I see, can you share what is preventing it from being merged?

Thanks!

[sazzad16]

@asolimando We both agreed on the solution but couldn't agree on what should be the default behavior. There were no one else for the tie-break. So this PR got stuck.

[asolimando]

@asolimando We both agreed on the solution but couldn't agree on what should be the default behavior. There were no one else for the tie-break. So this PR got stuck.

Thanks for your reply, when you say default behavior you mean if we need to add a new constructor so that we always pass the executor, or if we rely on the MultiNodePipelineBase#setExecutorService method (defaulting to the current implementation if)?

In our downstream version of this patch we went for the additional parameter in the constructor wherever needed, but honestly I wouldn't mind if the current patch goes in as-is and we rework our code to set the executor via MultiNodePipelineBase#setExecutorService, so if that helps breaking the tie, for me the patch could go in as it is now.

[yangbodong22011]

In our downstream version of this patch we went for the additional parameter in the constructor wherever needed

@sazzad16 @asolimando if you all agree with this change, I can revise this PR.

[killergerbah]

I would love to see this functionality eventually merged upstream because I noticed performance issues with MultiNodePipelineBase.sync. However it does seem like a code smell to use a static setter that affects the behavior of all instances of the class. Why not implement a setter on JedisCluster instead and supply the executor through the constructor of MultiNodePipelineBase?

[killergerbah]

Apologies for the continued unsolicited feedback but because the choice of default behavior here does seem to have major performance implications, I am going to offer it anyway:

I would argue that the most reasonable default behavior is to not try to perform the connection reads in parallel AT ALL. The original code attempts to do this by creating a thread pool for every call to sync, which is very expensive, but this PR attempts to give all the MultiNodePipelinebase instances the same thread pool, which runs the risk of thread pool starvation. Meanwhile, reading from the connections synchronously does not seem to make sync significantly slower - in fact, in my own tests locally it seems to go slightly faster most of the time.