Adds initial support for Spring for Apache Pulsar.
Work remaining: - [x] docs
Comment From: onobc
I have also added support for the following:
- Pulsar connection details in auto-config
- Pulsar service connection from testcontainers
- Pulsar docker compose support + service connections from docker compose
These changes are in my fork (based off this same PR branch) in 3 separate commits, queued up and ready to PR once this base PR is merged: - https://github.com/onobc/spring-boot/tree/cbono-add-pulsar-service-connection
Comment From: vpavic
Is this expected to target Spring Boot 3.2 as hinted in this blog post? I'm asking because this is still in 3.x milestone and work on 3.2 is well underway.
Comment From: onobc
Thanks for the detailed review @wilkinsona . From a quick read, the requested changes seem light in nature and all make sense. I will get this updated over the next couple of days.
Comment From: onobc
@wilkinsona I believe I have addressed all of your points except Please add Kotlin versions of code examples for the docs. Would you be ok w/ me adding that in a subsequent smaller PR?
Comment From: onobc
Thanks for the latest updates, @onobc.
You're welcome @wilkinsona. I will make the updates requested shortly.
In addition to my specific comments, I've also spent some time reviewing the configuration properties again. Unfortunately, I'm quite concerned about the sheer number of them.
Looking at the generated JSON, there are 219
spring.pulsarproperties. To put that into context, there are 1770 properties in total inspring-boot-autoconfigurewith 12% of those being for Spring Pulsar. Put another way, adding Spring Pulsar support increases the properties inspring-boot-autoconfigureby 14%.Maybe it's just the nature of the beast (there are 174
spring.kafka.*properties) but I do think it would be worth spending some time to see if the configuration model can be simplified. I think that would be of benefit both to us from a maintenance perspective and to users from an ease-of-use perspective.
I agree, there are many properties. I think it is the nature of the beast though. I also agree simplifying would be beneficial. However, I have attempted to do this but do not see a clear-cut path forward.
Comment From: onobc
@wilkinsona I have covered all of the recent concerns other than:
- too many modules (this will require change in Pulsar)
- too many config props (I don't see a way to reduce this currently)
- don't expose imperative components in reactive app (big effort to do this)
- Add Kotlin examples to docs (would like to do in subsequent PR)
Thanks
Comment From: philwebb
I find the number of configuration properties quite a concern too. I wonder if we can just add the properties that we think will be popular and lean on PulsarClientBuilderCustomizer for the more exotic ones?
One thing we've learnt from previous features is it's easier to add properties later when they are needed than take them away because they aren't.
Comment From: wilkinsona
We discussed the properties in a team meeting yesterday (without Phil, he was on PTO) and came to the same conclusion. @snicoll reminded my that our Kafka support has got to where it is having evolved over many releases. This makes its large number of properties somewhat easier to justify. Our feeling is that we shouldn't jump straight into such a position with Pulsar and should instead provide plenty of room for things to evolve based on community feedback.
As Phil suggests, the room to evolve can be provided by using one or more customizer callbacks to configure things not covered by configuration properties. It then becomes a question of what should be a configuration property. We'd defer to you on that as we lack the Pulsar expertise to have an informed opinion. As a general rule of thumb, settings that change from environment to environment (dev, staging, prod and so on) are the strongest candidates for configuration properties. Next would be settings that are really commonly tuned. Ideally we'd see a 5-10x reduction in the number of properties that we offer to begin with.
Comment From: onobc
Thanks for the latest upgrades, @onobc. The main two points still to address are SSL and the configuration properties. Apologies again for only identifying SSL now. Hopefully there isn't anything else major that's still lurking.
No worries on the SSL @wilkinsona - it is a big PR, there are many moving pieces - really thank you for the thorough reviews.
Also, I think it is a good thing to reduce the config props - the user still can get at any "switch" they need via the customizer (as pointed out by yourself and @philwebb ). Then the community will ask for what they want to be a property. I will sync w/ @sobychacko and we will come up w/ the reduced list.
Comment From: onobc
We took a first pass and were able to knock it down from ~ 196 -> 136 which is about a 30% reduction.
We also verified that for all of the properties we are removing, the user has a way to customize them (we are good on that front).
NOTE This has not been committed yet - just wanted to run the numbers by the team while the tests are being updated etc..
Data below
NOTE: the Admin and Client have 8 + 12 and 30 + 14, respectively. These are the regular + ssl props. I believe the SSL related ones may go away when we move to SSL bundles but I am not 100% sure so I left it like this for now so we can see the count w/ and w/o the SSL props.
| PulsarProperties | Before | After |
|---|---|---|
| Admin | 8 + 12 | 8 + 12 |
| Cache | 3 | 3 |
| Client | 30 + 14 | 7 + 13 |
| Defaults | 1 | 1 |
| Function | 3 | 3 |
| Listener | 6 | 6 |
| Reader | 7 | 7 |
| Template | 1 | 1 |
| ConsumerConfigProps | 36 | 26 |
| ProducerConfigProps | 27 | 17 |
| Total | 122 | 79 |
| ReactivePulsarProperties | Before | After |
|---|---|---|
| Cache | 3 | 3 |
| Consumer | 34 | 27 |
| Listener | 3 | 3 |
| Reader | 8 | 8 |
| Sender | 26 | 16 |
| Total | 74 | 57 |
| Original Total | New Total |
|---|---|
| 196 | 136 |
Which is about a 30% reduction.
The consumer and producer/sender config props (both imperative and reactive) are where most of the properties are. I am going to take another pass at those and see if we can drop it down further.
Comment From: onobc
@wilkinsona @philwebb config props reduced ~50% w/ the latest commit.
Data below
NOTE: the Admin and Client have 8 + 12 and 30 + 14, respectively. These are the regular + ssl props. I believe the SSL related ones may go away when we move to SSL bundles but I am not 100% sure so I left it like this for now so we can see the count w/ and w/o the SSL props.
| PulsarProperties | Before | After |
|---|---|---|
| Admin | 8 + 12 | 8 + 12 |
| Client | 30 + 14 | 7 + 13 |
| Defaults | 1 | 1 |
| Function | 3 | 3 |
| Listener | 6 | 6 |
| Reader | 7 | 6 |
| Template | 1 | 1 |
| ConsumerConfigProps | 36 | 15 |
| ProducerConfigProps | 27 | 12 |
| Total | 119 | 59 |
| ReactivePulsarProperties | Before | After |
|---|---|---|
| Consumer | 34 | 17 |
| Listener | 3 | 3 |
| Reader | 8 | 6 |
| Sender | 26 | 12 |
| Total | 71 | 38 |
| Original Total | New Total | % Reduction |
|---|---|---|
| 190 | 97 | 51.05% |