Introduce executor configuration to connection factories for Executor to be used with ClusterCommandExecutor #2669
Conversation
jxblum
added
in: lettuce
| @@ -51,6 +52,8 @@ public class RedisClusterConfiguration implements RedisConfiguration, ClusterCon | |||
|
|
|||
| private @Nullable Integer maxRedirects; | |||
|
|
|||
| private @Nullable AsyncTaskExecutor executor; | |||
There was a problem hiding this comment.
RedisClusterConfiguration doesn't seem ideal to hold the executor. Our Redis…Configuration classes are intended to describe the endpoint(s) to connect to and provide specific connectivity options such as the password or redirection limit that are used by the clients directly.
ClusterCommandExecutor is a Spring Data concept. The alternative of moving the executor into …ClientConfiguration seems also not ideal because the executor isn't required by the client.
How about adding executor as property to [Jedis|Lettuce]ConnectionFactory directly? We have a few properties already, that are factory-specific (convertPipelineAndTxResults, shareNativeConnection, pipeliningFlushPolicy)? While the executor is only required for clustering operations, this change would give us the opportunity to revise its purpose along the lines of a "general purpose executor for asynchronous activity such as running multiple commands concurrently".
What do you think?
There was a problem hiding this comment.
+1 for moving the executor to the connection factory.
Move executor from ClusterConfiguration to connection factories as the executor is a Spring concept that isn't tied to endpoint details or the client config. Reorder static factory methods after constructors and property accessors after static factory methods. Inline single-line single-use methods that aren't intended as extension hooks for easier readability. Remove NonNull annotations as default non-nullability is defined on the package level. Simplify tests to use integration tests to avoid excessive mocking. See #2594 Original pull request: #2669
Move executor from ClusterConfiguration to connection factories as the executor is a Spring concept that isn't tied to endpoint details or the client config. Reorder static factory methods after constructors and property accessors after static factory methods. Inline single-line single-use methods that aren't intended as extension hooks for easier readability. Remove NonNull annotations as default non-nullability is defined on the package level. Simplify tests to use integration tests to avoid excessive mocking. See #2594 Original pull request: #2669
Move executor from ClusterConfiguration to connection factories as the executor is a Spring concept that isn't tied to endpoint details or the client config. Reorder static factory methods after constructors and property accessors after static factory methods. Inline single-line single-use methods that aren't intended as extension hooks for easier readability. Disable TaskExecutor disposal on ClusterCommandExecutor.destroy(). Remove NonNull annotations as default non-nullability is defined on the package level. Simplify tests to use integration tests to avoid excessive mocking. See #2594 Original pull request: #2669
Move executor from ClusterConfiguration to connection factories as the executor is a Spring concept that isn't tied to endpoint details or the client config. Reorder static factory methods after constructors and property accessors after static factory methods. Inline single-line single-use methods that aren't intended as extension hooks for easier readability. Disable TaskExecutor disposal on ClusterCommandExecutor.destroy(). Remove NonNull annotations as default non-nullability is defined on the package level. Simplify tests to use integration tests to avoid excessive mocking. See #2594 Original pull request: #2669
|
That's merged and polished now. Going forward, we should limit our polishing commits not to outweigh the actual change and if we want to apply larger cleanups, then it is better to handle these as separate tickets that can be backported. I'm guilty here as well. |
|
@mp911de & @christophstrobl - I never had a chance to respond to the feedback on this PR & review, so I am not sure why we went ahead and merged this without completing the process. AFAICT, this was not time critical and certainly wasn't acted on with consensus. Rather, it seems we pushed this along to cram it into the release tomorrow ( IMO, the In general, the component that requires, refers to and uses a collaborator (for example, like the With respect to configuration, I don't understand what you mean by this statement:
Spring Data Redis is all client-side. There is no SD Redis for a Redis server, in the purest sense, which was something SD GemFire/Geode actually did support (i.e. SD GemFire/Geode could be and was used on the server-side). To my understanding, the Perhaps what would have been better in this case is a Strategy interface for how to "resolve" the "configured" The implementation of, say, the In any case, I don't agree with our current arrangement, and I think we should revisit this at some point. |
Closes #2594