I agree with the motivation. Just some minor suggestions:
Is AdvancedMessageId or MessageIdAdv better for this case to replace PulsarApiMessageId? It looks like LedgerHandleAdv to LedgerHandle in the bookkeeper. > We have to deprecated the BatchMessageAcker, which is just a wrapper of a BitSet and the batch size. Why do we need to deprecate the BatchMessageAcker? Not all users enable batch index ack. We are using BatchMessageAcker to track if all the messages of a batch have been acked. Thanks, Penghui On Sun, Dec 18, 2022 at 3:45 PM 丛搏 <bog...@apache.org> wrote: > < For a single-topic consumer, wrapping the topic name is > < redundant and might break the existing behavior. In this case, if > < `PulsarApiMessageId` extends `TopicMessageId`, the `getTopicName()` > < method should return null, which is not a good design [1][2]. > > For `TopicMessageIdImpl`, it is an original method. for > `PulsarApiMessageId` if extend `TopicMessageId` it is a new method for > any `MessageId` extend `PulsarApiMessageId`, why do we have to return > null? I think it just reduces the transmission of useless fields at > the network layer and not added to MessageIdData. LedgerId and EntryId > are in PulsarApiMessageId, why shouldn't `topicName` be added in? > > Thanks, > Bo > > Yunze Xu <y...@streamnative.io.invalid> 于2022年12月18日周日 14:23写道： > > > > Hi Bo, > > > > Because the topic name is not a part of MessageIdData. It's only used > > to find the correct internal consumer of a multi-topics consumer. > > > > > All I can think of is PulsarApiMessageId extend > TopicMessageId(PIP-224[1]) right? > > > > No. The `TopicMessageId` could only be used in a multi-topics > > consumer. For a single-topic consumer, wrapping the topic name is > > redundant and might break the existing behavior. In this case, if > > `PulsarApiMessageId` extends `TopicMessageId`, the `getTopicName()` > > method should return null, which is not a good design [1][2]. > > > > After both PIP-224 and PIP-229 are approved, the `TopicMessageIdImpl` > > will implement both `PulsarApiMessageId` and `TopicMessageId` > > interfaces. Other `MessageId` implementations only need to implement > > `PulsarApiMessageId`. > > > > BTW, PIP-224 mainly solves two problems: > > 1. When a multi-topics consumer acknowledges a `MessageId` that is not > > a `TopicMessageId`, a `PulsarClientException.NotAllowedException` will > > be thrown in synchronous methods. The asynchronous methods should not > > throw an exception. > > 2. For a multi-topics consumer, support seeking with a `TopicMessageId`. > > > > PIP-224 is designed for application users to specify an associated > > topic name when using a `MessageId` in `seek` or `acknowledge` on a > > multi-topics consumer. PIP-229 is more like a refactoring to allow the > > experienced developers access the fields of `MessageIdData` via a > > standard interface. > > > > [1] > https://github.com/apache/pulsar/issues/18616#issuecomment-1328609346 > > [2] https://lists.apache.org/thread/g8o0qtljllxnvck69dn36205xg5xr8cc > > > > Thanks, > > Yunze > > > > > > On Fri, Dec 16, 2022 at 8:50 PM 丛搏 <bog...@apache.org> wrote: > > > > > > Abstraction based on MessageIdData is a good solution. I don't have > > > any discussion context. Why don't we put the topic name in it? > > > > > > All I can think of is PulsarApiMessageId extend > > > TopicMessageId(PIP-224[1]) right? > > > > > > Thanks, > > > Bo > > > [1] https://github.com/apache/pulsar/issues/18616 > > > > > > Yunze Xu <y...@streamnative.io.invalid> 于2022年12月16日周五 15:59写道： > > > > > > > > Hi all, > > > > > > > > I've opened a PIP to discuss: > https://github.com/apache/pulsar/issues/18950 > > > > > > > > Currently the `MessageId` interface is not friendly to developers of > > > > Pulsar core and ecosystems. There is no abstraction of the > > > > `MessageIdData` defined in `PulsarApi.proto`. > > > > > > > > This proposal aims at solving this problem and allows more loose type > > > > assertions when using `seek` and `acknowledge`. > > > > > > > > You can also see the demo for reference: > > > > https://github.com/BewareMyPower/pulsar/pull/11 > > > > > > > > (Sorry I forgot to add the [DISCUSS] prefix again in the previous > > > > email, let's continue the discussion here) > > > > > > > > Thanks, > > > > Yunze >
