search

spring-projects / spring-boot

Spring Boot helps you to create Spring-powered, production-grade applications and services with absolute minimum fuss.
https://spring.io/projects/spring-boot
Apache License 2.0
75.22k stars 40.7k forks source link

General Feedback about Service Connections #34760

Closed eddumelendez closed 1 year ago

eddumelendez commented 1 year ago

First of all, thanks for introducing this feature. I already started using it in my examples

Also, sorry for writing all the way in this issue, but I think it would be much better to understand how to proceed instead of opening many issues/PRs

  1. 🐛 bug? The only examples I couldn't migrate were related to Yugabyte because of the driver. It's missing here

  2. 🆕 I would like to ask if you will introduce RedpandaServiceConnection. Redpanda offers a Kafka-compatible API, and it is very fast. I have almost everything to submit a PR, but I want to ask first :)

  3. 🔜 🤞🏽 When the time comes, I think adding support for Pulsar would be great. I just created an issue in their repo in case the autoconfiguration support in spring-boot takes longer https://github.com/spring-projects-experimental/spring-pulsar/issues/384

  4. ❓ What's the vision of ServiceConnections beyond spring-boot? I mean support in spring-cloud. I think this can be used in spring-cloud-consul, spring-cloud-vault, and spring-cloud-zookeeper. I ask because the Config Data approach offered by those projects could benefit from it. Currently, system properties needs to be added or registered through @ContextConfiguration(initializers = ...) which with ServiceConnections could possible be fixed too.

  5. ❓ Is RedisServiceConnection from org.springframework.boot.test.autoconfigure.data.redis instead of org.springframework.boot.test.autoconfigure.redis?. Asking for consistency reasons

  6. ❓ Given the current state of the art, can we introduce HazelcastServiceConnection expecting a GenericContainer similar to Redis? Given the approach to introducing those new integrations, the project should be ready to accept them if needed.

If 1, 2, 5, and 6 make sense to you, I can probably help with some or pair with someone else who wants to contribute. I guess a couple of those issues will be fixed by the team 😀

Once again, thank you so much for adding this feature.

quaff commented 1 year ago
  1. 🐛 bug? The only examples I couldn't migrate were related to Yugabyte because of the driver. It's missing here

Why would you configure jdbc driver manually? It's the responsibility of java.sql.DriverManager to load proper driver for specific jdbc url.

wilkinsona commented 1 year ago

This is great feedback. Thanks very much, @eddumelendez.

  1. Yes, this is a bug. Thanks for catching it. I have opened https://github.com/spring-projects/spring-boot/issues/34777
  2. We can certainly consider it. It feels like it's within scope given that Spring Boot supports Kafka and Testcontainers, and Testcontainers has a Redpanda module. If you have the code pretty much ready, please feel free to submit a PR. Alternatively, if you'd prefer not to spend any more time on it just yet, please open a separate issue that we can evaluate before potentially proceeding with a PR
  3. +1
  4. It's early days but we hope that service connections will be useful in Spring Cloud and beyond. For example, Spring Cloud Bindings may be able to define service connections rather than property sources.
  5. Our auto-configuration for Redis depends on Spring Data Redis as it uses org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory or org.springframework.data.redis.connection.jedis.JedisConnectionFactory. The property prefix (spring.data.redis) and package names are intended to reflect this dependency
  6. Hazelcast's an interesting one as we lean heavily on Hazelcast's configuration files and have very little in the way of our own configuration properties. It would be good to support service connections with Hazelcast, but I'm not yet sure how that should be implemented. I've opened https://github.com/spring-projects/spring-boot/issues/34778 so that we can start to explore things. If you have any suggestions, they'd definitely welcome on that issue.
eddumelendez commented 1 year ago

I will be submitting a PR for Redpanda :)

@wilkinsona thanks for the explanation! it all makes sense

eddumelendez commented 1 year ago

Before raising any PR. Does activemq and otlp service connections would be welcome? I think so but just wanted to confirm due to there is no specific testcontainers module but at least for otlp there is an official docker image.

wilkinsona commented 1 year ago

Thanks for the offer, @eddumelendez. We're aware of a few places where service connection support is missing. There's the two that you've already mentioned, Apache Artemis, and there may well be others. Pull requests would be welcome if you have the time. Thank you. Just to set expectations, with RC1 being released tomorrow, there's a high chance that any new support won't land until 3.2 now.

eddumelendez commented 1 year ago

Great! Yes, maybe we can create a list of those missing. You mean 3.1 next month, right?

So, probably this issue is already done. Please, feel free to close it and thanks for this feature again!

wilkinsona commented 1 year ago

After tomorrow's RC1 we will try not to add any new features. It may not be a complete freeze, but we generally only fix bugs, improve the documentation, and amend existing functionality in the RC phase. That means that any new service connection support may have to wait until work in 3.2 begins in the summer once 3.1 has shipped.

So, probably this issue is already done. Please, feel free to close it and thanks for this feature again!

Thanks for the really timely feedback and your PRs. They're much appreciated.