Setting subscription name using `spring.pulsar.consumer.subscription.name` configuration property doesn't work

[vpavic]

Pulsar Listener section of reference manual provides snippets that suggest it's possible to configure subscription name only using spring.pulsar.consumer.subscription.name configuration property and without having subscriptionName specified on @PulsarListener.

However, in practice, I don't see this being possible due to subscription name being generated in PulsarListenerAnnotationBeanPostProcessor if omitted on annotation:

spring-pulsar/spring-pulsar/src/main/java/org/springframework/pulsar/annotation/PulsarListenerAnnotationBeanPostProcessor.java

Lines 364 to 369 in 728a74b

	private String getEndpointSubscriptionName(PulsarListener pulsarListener) {
	if (StringUtils.hasText(pulsarListener.subscriptionName())) {
	return resolveExpressionAsString(pulsarListener.subscriptionName(), "subscriptionName");
	}
	return GENERATED_ID_PREFIX + this.counter.getAndIncrement();
	}

Which combined with DefaultPulsarConsumerFactory applying the generated subscription name to ConsumerBuilder results in the value specified by spring.pulsar.consumer.subscription.name configuration property being overwritten by generated one:

spring-pulsar/spring-pulsar/src/main/java/org/springframework/pulsar/core/DefaultPulsarConsumerFactory.java

Lines 86 to 88 in 728a74b

	if (subscriptionName != null) {
	consumerBuilder.subscriptionName(subscriptionName);
	}

[onobc]

Hi @vpavic - thanks for the report, I will take a look.

[onobc]

Good catch @vpavic, you are correct. This does not work as advertised because (as you detailed in the description) the default is only applied if the incoming name is null and in this case the annotation processor has set it to the generated name.

That being said, I don't think it would be valuable to use the default sub name because it is likely there will be multiple @PulsarListeners which would then require separate names. What I think would be helpful would be to allow the setting a default listener container sub name generated prefix and then the name would be MY_PREFIX(0...N). A more flexible step would be to introduce the concept of a subscription name generator and plug that into the listener/containers.

For now I will remove the falsey out of the docs and keep this issue open for further exploration around this area. Sound good to you?

Again, thanks for finding this and thanks for the detailed analysis.

[vpavic]

Thanks for looking into this @onobc, I'll review the PR that updates the documentation.

To give you more context about our use case - we're integrating with a 3rd party platform that exposes Pulsar-based message queue that is used to subscribe to events that occur within that platform. So we'll only have a single @PulsarListener where subscription name is based on our tenant id (the actual pattern is ${tenantId}-sub) and therefore should be externally configurable (because tenant ids will differ from environment to environment).

That's why the originally documented combination of @PulsarListener without subscription name coupled with spring.pulsar.consumer.subscription.name would be ideal for our use case.

I should note that there are also some workarounds possible, such as @PulsarListener(subscriptionName = "${spring.pulsar.consumer.subscription.name}") - this is not pretty but it works around the issue and allows subscription name to be managed using properties.

Other solution is to use ConsumerBuilderCustomizer and supply subscription name there, but this still needs the customizer to be referred to from the annotation @PulsarListener(consumerCustomizer = "myConsumerBuilderCustomizer"). This approach is actually quite similar to the concept of a subscription name generator that you mentioned. The only improvement I'd like to see is that if there's only one ConsumerBuilderCustomizer bean available in the context it could be applied implicitly, without the need to refer to it in @PulsarListener.

[onobc]

That's why the originally documented combination of @PulsarListener without subscription name coupled with spring.pulsar.consumer.subscription.name would be ideal for our use case.

Ahh, good to know this use case.

I should note that there are also some workarounds possible, such as @PulsarListener(subscriptionName = "${spring.pulsar.consumer.subscription.name}")

Ha! I was going to point to this workaround now as well. I would go w/ this one over the customizer for your particular case.

The only improvement I'd like to see is that if there's only one ConsumerBuilderCustomizer bean available in the context it could be applied implicitly, without the need to refer to it in @PulsarListener.

Also, what if there are multiple PulsarListeners - would we want to auto-apply the single customizer to all PLs? Either way, I could see some users not wanting the auto-apply in this case. This would lead us to having a switch to disable the auto-apply.

I might introduce the subscriptionName generator concept and auto-apply it to all listeners if it is provided in the context otherwise go w/ the default generator. This will be low risk (additive) and will solve your case.

[vpavic]

I understand the reasoning for not auto-applying ConsumerBuilderCustomizer.

I might introduce the subscriptionName generator concept and auto-apply it to all listeners if it is provided in the context otherwise go w/ the default generator. This will be low risk (additive) and will solve your case.

This sounds good to me. If you decide that you definitely want to go this route, I'd be willing to contribute.

[onobc]

I'd be willing to contribute.

I wish I would have checked this comment a few hours ago as I am about 95% done w/ implementing it :)

I will add tests later today or tomorrow and then submit the PR and you can code review it if you don't mind.

[vpavic]

No worries, if you were able to work on this immediately it's probably better that way because it would take at least a week or so for me to find time to dedicate to this.

There will be opportunities for other contributions I'm sure, as this 3rd party platform that we're integrating with over Pulsar is quite a substantial part of one of our current projects. 🙂

Anyway, feel free to ping me when you have the PR ready, I'll be glad to take a look.