[feat][client] PIP-224 Part 2: Add TopicMessageIdSerDes#20
Open
BewareMyPower wants to merge 4 commits intomasterfrom
Open
[feat][client] PIP-224 Part 2: Add TopicMessageIdSerDes#20BewareMyPower wants to merge 4 commits intomasterfrom
BewareMyPower wants to merge 4 commits intomasterfrom
Conversation
Master Issue: apache#18616 ### Motivation Introduce `TopicMessageId` to support getting the owner topic of a `MessageId`. When a `MessageId` is retrieved from a received message, the owner topic will be correctly set by the client library. When it's returned by `Producer#send`, this PR provides a `TopicMessageId#create` method to configure the owner topic. `acknowledge` APIs are affected only for the error cases: when a `MessageId` other than a `TopicMessageId` is accepted on a multi-topics consumer, `PulsarClientException.NotAllowedException` will be thrown. The semantic of the `seek(MessageId)` API is changed. Now if a `TopicMessageId` is accepted on a multi-topics consumer, the seek behavior will happen on the internal consumer of the owner topic. ### Modifications - Add the `TopicMessageId` interface. - In `MultiTopicsConsumerImpl#doAcknowledge`, complete the future with `NotAllowedException` if the argument is not a `TopicMessageId`. - In `MultiTopicsConsumerImpl#seekAsync`, when the argument is a `TopicMessageId`, find the internal consumer according to the owner topic and pass the argument to it if it exists. - In `ConsumerImpl#seekAsync`, get the inner message ID of the `TopicMessageId` so that now a single-topic consumer can also accept a `TopicMessageId` to seek. Besides the main modifications above, this patch does some refactorings to avoid direct access to `TopicMessageIdImpl`: - Deprecated `getTopicName` method by trimming the partition suffix of the owner topic in `getOriginTopicNameStr`. - Deprecated `getTopicPartitionName` by `getOwnerTopic`. - `getInnerMessageId` cannot be deprecated because we still need to convert `TopicMessageId` to `MessageIdImpl` in many cases (because we cannot get the fields like ledger id). Instead of deprecating it, use `MessageIdImpl.convertToMessageIdImpl` to replace it. - In `convertToMessageIdImpl`, for a customized `TopicMessageId` implementation, use serialization and deserialization to get the `MessageIdImpl` object. ### Verifications Add the following tests to `MultiTopicsConsumerTest`: - `testAcknowledgeWrongMessageId`: verify the correct exceptions are thrown in `acknowledge` APIs - `testSeekCustomTopicMessageId`: verify the new seek semantics for a `TopicMessageId`, including the existing `TopicMessageIdImpl` and the customized implementation by `TopicMessageId#create` ### TODO - Add a standard SerDes class for `TopicMessageId` - Apply `TopicMessageId` into `getLastMessageId` related APIs. - Deprecate the `getInnerMessageId` after PIP-229 is approved.
|
The pr had no activity for 30 days, mark with Stale label. |
rdhabalia
suggested changes
Apr 3, 2023
rdhabalia
left a comment
rdhabalia
left a comment
There was a problem hiding this comment.
serializing and deserializing is expensive and having different APIs for different use cases are creating a really bad experience for users and I strongly feel we should avoid such APIs and complexity if things can be solved with a simple straight forward change with the same API and without creating bad user experience.
I think we should consider this simple PR without costing performance and API compatibility and confusing usage to the users:
apache#19944
Open
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Master issue: apache#18616