-
Couldn't load subscription status.
- Fork 3.7k
[improve][pip] PIP-445: Add Builder Methods to Create Message-based TableView #24842
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
base: master
Are you sure you want to change the base?
Conversation
|
@namest504 Please add the following content to your PR description and select a checkbox: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A different approach should be taken. Please check the comment #24809 (review) .
The current getRawMessage solution would be a breaking change for existing TableView API users since it would unnecessarily consume more memory and CPU. That's why a different approach must be taken. I've suggested to resolve the issue by adding new methods to the API for returning a TableView with Message as the type parameter.
TableView<Message<T>> createForMessages() throws PulsarClientException;
CompletableFuture<TableView<Message<T>>> createForMessagesAsync() throws PulsarClientException;Would that approach solve your use case?
|
@namest504 Please subscribe to the dev mailing list using the instructions available at https://pulsar.apache.org/contact/#mailing-lists unless you already have completed the steps. (I released your previous email from the moderation queue and emails from out-of-list addresses usually end up there.) Another detail is that you are currently anonymous. It's encouraged to work with a real name in Apache projects such as Apache Pulsar since larger contributions are made under contributor agreements and if you'd ever like to become a committer, you'd have to have a real identity. We don't ask contributors to sign a CLA for minor contributions so it's not a blocker in this case. |
b6cb0e7 to
52f47ac
Compare
@lhotari I've revised the PR title. I've updated the PR to adopt the builder-based approach you suggested. This new commit implements the Please let me know if this new direction looks good. Thanks again! |
|
Also, I've subscribed to the developer mailing list as you recommended. |
|
When I was working on the experiment, lhotari@c5a4991, one idea that came into mind is that it would be possible to allow creating a view based on a Function that maps the message to some other object. This would allow using a custom object that takes whatever properties are needed from the message properties and there wouldn't be a need to keep a reference to the complete message instance. |
|
Here's the MessageMapperTableViewImpl experiment: for the builder, there would be these methods (instead of the createForMessages) <V> TableView<V> createMapped(Function<Message<T>, V> mapper, boolean releasePooledMessage) throws PulsarClientException;
<V> CompletableFuture<TableView<V>> createMappedAsync(Function<Message<T>, V> mapper, boolean releasePooledMessage) throws PulsarClientException;with this, you could create a @namest504 I think that this mapper function based approach would provide more flexibility for different use cases where message properties would need to be taken into account. Do you see any problems with this approach? |
|
Thank you for the continued guidance. I want to confirm my understanding and share my thoughts before proceeding with the implementation. My understanding is that the goal here is to provide maximum flexibility—evolving beyond a binary choice of payload vs raw message and instead empowering users to transform a Message into any custom object V that fits their specific needs. Is this correct? I agree this is a very appropriate and powerful extension for this feature. For example public class OriginalData {
String a;
double b;
}
public class Foo {
String a;
double b;
long eventMetaA; // from message
String eventMetaB; // from message
}
TableView<Foo> view = builder
.createMapped(message -> {
OriginalData original = message.getValue();
return new Foo(
original.a,
original.b,
message.getEventMetaA(),
message.getEventMetaB()
);
});I do have two main concerns regarding potential edge cases, and I would appreciate your guidance on them.
Are these concerns that I should address in the implementation, or are they handled by other parts of the system and not something I need to consider? If my understanding of the direction is correct, I'd like to update the PIP document and the implementation PR accordingly. Please let me know if this is the right way to proceed. |
In that case, the key is removed from the map since null is considered to be a tombstone value.
Good question. I don't think that this PIP impacts this. If the mapper throws an exception, the same message will get retried, so the table view will get stuck. The table view doesn't have any way to handle "poison pill" messages which fail every time.
Makes sense, please go ahead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please go ahead with the voting for PIP-445
Review comments were addressed. Approval will be made after a successful vote on the dev mailing list.
Please update the summary in the description of the PR |
This PR introduces PIP-445, which proposes adding a
getRawMessage()method to the TableView API.This allows users to access the full message metadata (e.g., properties, event time), not just the payload.
Implementation PR: #24809
mailing list discussion thread: https://lists.apache.org/thread/12jxo0n3njjpm17w2zdo2qvx1bs3y2dk
docdoc-requireddoc-not-neededdoc-complete