Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage-related auto-configurations must be ordered after the auto-configurations they rely upon #51

Closed
snicoll opened this issue Oct 13, 2020 · 8 comments

Comments

@snicoll
Copy link
Contributor

snicoll commented Oct 13, 2020

Describe the bug
Various auto-configurations are reacting to the fact another bit of infrastructure is present. For instance, the elasticsearch one checks if a HighLevelClient is defined. Yet, the auto-configuration does not declare anywhere it should be processed once the auto-configuration for elasticsearch has been processed.

See @AutoConfigureBefore, @AutoConfigureAfter for more details.

Environment
n/a

rdehuyss added a commit that referenced this issue Oct 13, 2020
@rdehuyss
Copy link
Contributor

Hey @snicoll ,

thanks for your valuable input. I'm a bit lost of what to do with the Lettuce and Redis related autoconfiguration.

I see the LettuceConnectionConfiguration but it does not export a RedisClient bean. Same goes for the JedisConnectionConfiguration which does not export a JedisPool bean. Do you know in which configuration I can find them or am I missing something?

@snicoll
Copy link
Contributor Author

snicoll commented Oct 14, 2020

As you've noticed yourself, we don't export those. If your code is relying on that, users will have to create the bean.

@mp911de
Copy link

mp911de commented Oct 14, 2020

Jedis and Lettuce drivers use different client implementations (RedisClient/Jedis/JedisPool/JedisSentinelPool vs. RedisClusterClient/JedisCluster) depending on the mode of operation (standalone, Sentinel, Cluster). That makes the actual client object an implementation detail of Spring Data Redis' RedisConnectionFactory. Exposing these details makes probably sense to some extent (e.g. via JedisConnectionFactory/LettuceConnectionFactory.getNativeClient) but it doesn't make sense to expose these objects as beans since their lifecycle is tied to RedisConnectionFactory.

If we consider an arrangement without Spring Data Redis, then client object beans such as RedisClient would make sense on their own as that would be the primary interaction API.

@rdehuyss
Copy link
Contributor

If we consider an arrangement without Spring Data Redis, then client object beans such as RedisClient would make sense on their own as that would be the primary interaction API.

This is indeed how it is done for others like the MongoDB driver.

Are you guys planning to do it also for the Lettuce/Jedis?

@snicoll
Copy link
Contributor Author

snicoll commented Oct 14, 2020

The team is not planning to do this at this time.

@mp911de
Copy link

mp911de commented Oct 14, 2020

Part of the rationale is that configuring the right client correctly is tied to a lot of complexity (Pooling vs. non pooling, Standalone, Sentinel, Cluster, Master/Replica operation modes) and that complexity is already encapsulated in Spring Data Redis.

@rdehuyss
Copy link
Contributor

Part of the rationale is that configuring the right client correctly is tied to a lot of complexity (Pooling vs. non pooling, Standalone, Sentinel, Cluster, Master/Replica operation modes) and that complexity is already encapsulated in Spring Data Redis.

I completely understand - supporting all the different data stores in JobRunr, also took me quite some time to understand how each client library uses pooling/ non-pooling/ ... .

@rdehuyss
Copy link
Contributor

I'll then close this issue as the requested change is done for the other providers (ElasticSearch, MongoDB and Sql DataSource)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants