Skip to content

Clarify purpose of Pulsar consumer subscription related properties #42053

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

Closed
vpavic opened this issue Aug 28, 2024 · 11 comments
Closed

Clarify purpose of Pulsar consumer subscription related properties #42053

vpavic opened this issue Aug 28, 2024 · 11 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@vpavic
Copy link
Contributor

vpavic commented Aug 28, 2024

Some while ago I opened these two issues against the Spring Pulsar project:

After setting up another project recently that uses Spring Pulsar for consuming messages, I've again tripped up on the same thing attempting to set Pulsar consumer subscription related settings using Spring Boot provided configuration properties.

Though Spring Pulsar reference manual now contains the following two tips:

The spring.pulsar.consumer.subscription.name property is ignored and is instead generated when not specified on the annotation.

The spring.pulsar.consumer.subscription-type property is ignored and is instead taken from the value on the annotation. However, you can set the subscriptionType = {} on the annotation to instead use the property value as the default.

IMO it would be good to revisit these properties as their purpose is (at least to me) unclear at the moment.

/cc @onobc

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 28, 2024
@onobc
Copy link
Contributor

onobc commented Aug 29, 2024

Thanks for the report @vpavic ,

subscription-name

The spring.pulsar.consumer.subscription-name is only used as a default subscription name when using the consumer factory directly. However, I expect about 99% of users will not use the consumer factory directly.
Another couple of downsides to setting a default sub name:

  • As a static property it is really not valuable since it will likely vary by each consumer. More appropriate would be a subscription-name-expression that could resolve to something sensical.
  • Subscription name is really important in Pulsar and leaving this to a default property could lead to problems if not used properly.

Based on the above downsides I would be in favor of deprecating (for removal) this property in 3.4.0. @philwebb wdyt?

subscription-type

The spring.pulsar.consumer.subscription-type is used as a default subscription name when using the consumer factory directly OR when using PulsarListener and setting thesubscriptionType = {}.

Note

But this is an opt-in behavior as IFF the user does @PulsarListener(subscriptionType = {}) does the spring.pulsar.consumer.subscription-type property get respected as the default.

It may be better suited if we set the default on PulsarListener(subscriptionType = {}) and then set the default spring.pulsar.consumer.subscription-type=Exclusive. This would still keep the default of Exclusive but would now let the SB config prop drive the system default.

Thoughts? @wilkinsona @vpavic ?

onobc added a commit to onobc/spring-pulsar that referenced this issue Aug 29, 2024
This commit moves the default subscription type from the
`@PulsarListener` annotation to the container factory (props) which
allows the `spring.pulsar.consumer.subscription-type` config prop to
be respected.

See spring-projects/spring-boot#42053
@vpavic
Copy link
Contributor Author

vpavic commented Aug 29, 2024

The spring.pulsar.consumer.subscription-name is only used as a default subscription name when using the consumer factory directly.

The spring.pulsar.consumer.subscription-type is used as a default subscription name when using the consumer factory directly

I wasn't aware of this. Are there any examples of how spring.pulsar.consumer.subscription.name and .subscription.type come into play with consumer factory driven configuration?

To clarify my use case - I'm integrating a 3rd party system that exposes a Pulsar topic as a means of subscribing to events published by that system. So nothing Pulsar related is in my control and key settings that impact the listener (topic and subscription names) are changing between different environments. So I'd like to be able to control those solely using configuration properties. Current setup/workaround involve using expressions in @PulsarListener attributes but I'd like to avoid that if possible.

It may be better suited if we set the default on PulsarListener(subscriptionType = {}) and then set the default spring.pulsar.consumer.subscription-type=Exclusive. This would still keep the default of Exclusive but would now let the SB config prop drive the system default.

That sounds good, but I assume that configuration facilities behind @EnablePulsar would also have to take care that Exclusive stays as default if not specified on the annotation (for scenarios that don't involve Spring Boot and its auto-configuration).

@philwebb
Copy link
Member

I don't think any of us the Boot team have enough experience of Pulsar to give deep advice, but on the surface both changes seem sensible. We can certainly do them in 3.4.

@onobc Do you want to raise issues/PRs for them?

@onobc
Copy link
Contributor

onobc commented Aug 29, 2024

I wasn't aware of this. Are there any examples of how spring.pulsar.consumer.subscription.name and .subscription.type come into play with consumer factory driven configuration?

There are no examples, but rather the other higher-level abstractions (the containers used by @PulsarListener, and @PulsarReader, @ReactivePulsarListener`) use the consumer factories. It is expected to be an edge case to use them directly.

That sounds good, but I assume that configuration facilities behind @EnablePulsar would also have to take care that Exclusive stays as default if not specified on the annotation (for scenarios that don't involve Spring Boot and its auto-configuration).

Yes, the default would still be exclusive w/ or w/o using Spring Boot. Although this would only help w/ subscriptionType and it sounds like ultimately you want to be able to specify topic and subscription name using config props.

So I'd like to be able to control those solely using configuration properties. Current setup/workaround involve using expressions in @PulsarListener attributes but I'd like to avoid that if possible.

These properties are only defaults to be used when the user does not specify them on the consumer.
Assuming you have more than 1 topic/sub in the application you would need the property to contain a SpEL that would be resolved at listener container creation time.

Do you mind providing 2 sample PulsarListener signatures you are currently using to solve this? I want to be sure I am on the same page. Thank you @vpavic

Something like:

@PulsarListener(topic = "${env}-topic-1", sub = "sub-${env}-topic1")
void listen1(String msg)

@PulsarListener(topic = "${env}-topic-2", sub = "sub-${env}-topic2")
void listen2(String msg)

@vpavic
Copy link
Contributor Author

vpavic commented Aug 29, 2024

Assuming you have more than 1 topic/sub in the application you would need the property to contain a SpEL that would be resolved at listener container creation time.

That's the point - there's only a single topic/subscription. From my experience, that isn't an uncommon pattern. Over the years I've seen quite a few systems that expose a single subscribable interface (basically a topic) that's then used to receive different messages. Anyway, to be more specific in my case it's Tuya - you can read more details about this in their developer documentation.

What I currently have is basically this (my.other.prop is environment specific):

spring.pulsar.client.authentication.plugin-class-name=my.custom.Authentication
spring.pulsar.consumer.topics=${my.other.prop}/out/event
@PulsarListener(subscriptionName = "${my.other.prop}-sub", subscriptionType = SubscriptionType.Failover)
void receive(byte[] message) {
	// process the message
}

@onobc
Copy link
Contributor

onobc commented Aug 29, 2024

Thanks @vpavic - the single topic/sub simplifies things greatly.

If you could have these properties:

spring.pulsar.client.authentication.plugin-class-name=my.custom.Authentication
spring.pulsar.consumer.topics=${my.other.prop}/out/event
spring.pulsar.consumer.subscription.name=${my.other.prop}-sub
spring.pulsar.consumer.subscription.type=Failover

and this listener:

@PulsarListener
void receive(byte[] message) {
	// process the message
}

then you would be good. Correct?

The good news is that topic property is already respected and I am adding support for subscription.type here. The final piece will be to do something similar for subscription.name next.

onobc added a commit to onobc/spring-pulsar that referenced this issue Aug 30, 2024
This commit moves the default subscription type from the
`@PulsarListener` and `@ReactivePulsarListener` annotation to the
associated container factory (props) which allows the Spring Boot
`spring.pulsar.consumer.subscription-type` config prop to be respected.

See spring-projects/spring-boot#42053
onobc added a commit to spring-projects/spring-pulsar that referenced this issue Aug 30, 2024
This commit moves the default subscription type from the
`@PulsarListener` and `@ReactivePulsarListener` annotation to the
associated container factory (props) which allows the Spring Boot
`spring.pulsar.consumer.subscription-type` config prop to be respected.

See spring-projects/spring-boot#42053
@vpavic
Copy link
Contributor Author

vpavic commented Aug 30, 2024

This sounds like what I'd ideally like to have, thanks.

So it looks like deprecation of spring.pulsar.consumer.subscription.name you mentioned in #42053 (comment) is off the table, right?

In any case, let me know if I can help with any of the work needed to deliver this, either on Spring Pulsar or Spring Boot side.

@onobc
Copy link
Contributor

onobc commented Aug 30, 2024

So it looks like deprecation of spring.pulsar.consumer.subscription.name you mentioned in #42053 (comment) is off the table, right?

Correct. After digging further and understanding your requirements ai bit more, leaving the subscription name here and giving it the same treatment we did for the subscription type makes sense. Also, the fact that the name can be dynamic w/ property substitution does make it more valuable than a static value (which would be pretty useless).

In any case, let me know if I can help with any of the work needed to deliver this, either on Spring Pulsar or Spring Boot side.
So very much appreciated @vpavic but I am already halfway through the name portion. The type portion merged yesterday. I expect no changes on the Boot side as we are leaving the properties as-is. I also plan on adding either a sample or IT on the framework side that exercises your use case.

Thank you for raising these issues!

@onobc
Copy link
Contributor

onobc commented Aug 30, 2024

The output of "Clarify purpose of Pulsar consumer subscription related properties" is to make them work as described in the config props. This will be done mostly in the Spring for Apache Pulsar project. First part is complete spring-projects/spring-pulsar#818. Remaining is in process and I will link the PR here once submitted.

There is a small change required on the Boot side to support subscription name. Submitting a PR for this soon.

onobc added a commit to onobc/spring-boot that referenced this issue Aug 30, 2024
The subscription name config prop was not being set on the Pulsar
listener container properties. This commit adds the subscription
name to the Pulsar property mappers.

Resolves spring-projects#42053
onobc added a commit to onobc/spring-pulsar that referenced this issue Aug 30, 2024
This commit moves the default subscription name from the
`@PulsarListener` annotation to the container factory (props) which
allows the `spring.pulsar.consumer.subscription.name` config prop to
be respected.

See spring-projects/spring-boot#42053
onobc added a commit to onobc/spring-pulsar that referenced this issue Aug 31, 2024
This commit moves the default subscription name from the
`@PulsarListener` and `@ReactivePulsarListener` annotation to the
corresponding container factory (props) which allows the
`spring.pulsar.consumer.subscription.name` config prop to be respected.

See spring-projects/spring-boot#42053
@onobc
Copy link
Contributor

onobc commented Aug 31, 2024

The final limitation was removed here - the docs no longer special-case the subscription name/type properties and config props can be used to configure the listeners.

onobc added a commit to spring-projects/spring-pulsar that referenced this issue Aug 31, 2024
This commit moves the default subscription name from the
`@PulsarListener` and `@ReactivePulsarListener` annotation to the
corresponding container factory (props) which allows the
`spring.pulsar.consumer.subscription.name` config prop to be respected.

See spring-projects/spring-boot#42053
@philwebb
Copy link
Member

Thanks @onobc! Closing this one in favor of the other issues/PRs.

@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2024
@philwebb philwebb added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 31, 2024
onobc added a commit to onobc/spring-pulsar that referenced this issue Sep 5, 2024
This commit adds an integration test to verify the following Spring Boot
config props can be used to configure `@PulsarListener` and
`@ReactivePulsarListener`:

- `spring.pulsar.consumer.topic`
- `spring.pulsar.consumer.subscription.name`
- `spring.pulsar.consumer.subscription.type`

See spring-projects/spring-boot#42053
onobc added a commit to spring-projects/spring-pulsar that referenced this issue Sep 6, 2024
This commit adds an integration test to verify the following Spring Boot
config props can be used to configure `@PulsarListener` and
`@ReactivePulsarListener`:

- `spring.pulsar.consumer.topic`
- `spring.pulsar.consumer.subscription.name`
- `spring.pulsar.consumer.subscription.type`

See spring-projects/spring-boot#42053
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
4 participants