Skip to content

Add Spring Pulsar transaction support #40189

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
wants to merge 3 commits into from

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Apr 8, 2024

Adds auto-config for Spring for Apache Pulsar transactions.

Introduces new txn config props for template and listener under the spring.pulsar.template.transaction and
spring.pulsar.listener.transaction prefixes, respectively.

Disables transaction support by default (opt-in feature).

Note

I still need to update/add docs for this but wanted to get this front-loaded as I will be out of pocket at DevNexus this week.

TODO

  • Update ref docs w/ info about transaction support

Adds auto-config for Spring for Apache Pulsar transactions.

Introduces new txn config props for template and listener under the
`spring.pulsar.template.transaction` and
`spring.pulsar.listener.transaction` prefixes, respectively.

Disables transaction support by default (opt-in feature).
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 8, 2024
@onobc onobc requested a review from wilkinsona April 15, 2024 16:44
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rebased, squashed, and polished the changes. While doing so, I had a bit more time to think about the .enabled and .required properties that can be configured in a way that's invalid. I still don't like it, I'm afraid.

With the .enabled and .required properties, it feels like one configuration option has been split across two settings.

I wonder if we could have a single property backed by an enum instead. The enum's values, something like DISABLED, SUPPORTS, and REQUIRES, would map to the current properties like this:

.enabled .required
DISABLED false false
SUPPORTS true false
REQUIRES true true
false true

With no value for the invalid .enabled=false .required=true combination, we've hopefully made things easier to configure.

I wonder if Spring Pulsar would also benefit from such a change as it too has enabled and required properties. If so, it could then also provide the enum that Boot exposes as a configuration property.

@onobc
Copy link
Contributor Author

onobc commented Apr 17, 2024

properties that can be configured in a way that's invalid. I still don't like it, I'm afraid.

Not a problem. Thanks for the focus on this. This is the perfect time to get it right (before people start using it).

With the .enabled and .required properties, it feels like one configuration option has been split across two settings.

Agreed.

I wonder if we could have a single property backed by an enum instead. The enum's values, something like DISABLED, SUPPORTS, and REQUIRES, would map to the current properties like this:

I like this approach.

With the .enabled and .required properties, it feels like one configuration option has been split across two settings.

Absolutely.

Based on the fact that we will want the enum to exist in Spring Pulsar and it has already cut the RC1 release, are you ok if we get this in during RC phase (like early next week) ?

@philwebb
Copy link
Member

Based on the fact that we will want the enum to exist in Spring Pulsar and it has already cut the RC1 release, are you ok if we get this in during RC phase (like early next week) ?

I don't think we should attempt to add this to Spring Boot 3.3 if we can't get something in before our RC1. Perhaps I can take a look today and add the enum on our side then just make a minor amendment post RC to migrate the enum. That will at least give folks a chance to try it before we GA.

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 17, 2024
@philwebb philwebb added this to the 3.3.x milestone Apr 17, 2024
@onobc
Copy link
Contributor Author

onobc commented Apr 17, 2024

Perhaps I can take a look today and add the enum on our side then just make a minor amendment post RC to migrate the enum. That will at least give folks a chance to try it before we GA.

That would be excellent @philwebb . If you can't get to it, let me know and I would be happy to add it to the Boot side as well. I am already in progress on adding it to the Spring Pulsar side (which we would make the minor amendment post RC to switch the enum that is used).

philwebb added a commit that referenced this pull request Apr 18, 2024
Adds auto-config for Spring for Apache Pulsar transactions.

Introduces a new `spring.pulsar.transaction.enabled` property
which can be used to enable transactions. This feature is
opt-in and remains disabled by default.

See gh-40189

Co-authored-by: Andy Wilkinson <[email protected]>
Co-authored-by: Phillip Webb <[email protected]>
@philwebb philwebb closed this in d55eb5b Apr 18, 2024
@wilkinsona wilkinsona modified the milestones: 3.3.x, 3.3.0-RC1 Apr 18, 2024
@onobc onobc deleted the cbono-add-pulsar-txns branch May 28, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants