Skip to content
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

[improve][client] Add newMessage with schema and transactions #23942

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

omarkj
Copy link
Contributor

@omarkj omarkj commented Feb 6, 2025

Pulsar Client allows callers to create messages with a schema or a transaction, but not both.

This commit adds a new method in the producer that allows callers to create a message with both a schema and transaction.

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Feb 6, 2025
@dao-jun
Copy link
Member

dao-jun commented Feb 7, 2025

@omarkj
Copy link
Contributor Author

omarkj commented Feb 7, 2025

PIP PR filed here: #23950, will follow those guidelines by emailing out to the thread soon, etc.

Pulsar Client allows callers to create messages with a schema or a transaction, but
not both. This commit adds a new method in the producer that allows callers to create
a message with both a schema and transaction.
@omarkj omarkj force-pushed the producer-new-message-schema-txn branch from d77ad02 to 59dd60a Compare February 10, 2025 19:16
@omarkj omarkj requested a review from BewareMyPower February 10, 2025 22:35
@lhotari lhotari added this to the 4.1.0 milestone Feb 28, 2025
@lhotari lhotari added area/client type/feature The PR added a new feature or issue requested a new feature area/transaction labels Feb 28, 2025
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari
Copy link
Member

lhotari commented Feb 28, 2025

@BewareMyPower @dao-jun The PIP has been approved and merged, https://github.com/apache/pulsar/blob/master/pip/pip-407.md . PTAL and review

Comment on lines +147 to +148
<V> TypedMessageBuilder<V> newMessage(Schema<V> schema,
Transaction txn);
Copy link
Member

Choose a reason for hiding this comment

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

I have a concern about backward compatibility with this interface change. Adding a new method without a default implementation will break source compatibility for any custom implementations of the Producer interface.
While it might be uncommon for users to implement this interface directly, it could happen in testing scenarios or custom proxy implementations. Java source compatibility rules require all implementors to provide all interface methods.

We could address the source compatibility issue by adding a default implementation:

Suggested change
<V> TypedMessageBuilder<V> newMessage(Schema<V> schema,
Transaction txn);
default <V> TypedMessageBuilder<V> newMessage(Schema<V> schema, Transaction txn) {
// to retain source compatibilty of Producer interface for implementors of the Producer interface
throw new UnsupportedOperationException(
String.format("Method not implemented in: %s", getClass().getName()));
}

The benefit of making it source compatible is that we could also backport this change to the 4.0.x client without introducing breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client area/transaction doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants