Hi Penghui,
It makes sense to me. I updated the proposal and added the API docs to
the `MessageIdAdv`, as well as why will `BatchMessageAcker` be
deprecated.
Thanks,
Yunze
On Mon, Dec 19, 2022 at 12:57 PM PengHui Li <peng...@apache.org> wrote:
>
> Hi Yunze,
>
> Thanks for the explanation. Now I understand why the BatchMessageAcker will
> be deprecated.
> For the interface face. I just thought they were for different users.
>
> For common users, they can only use MessageID.
> For advanced users (Pulsar ecosystem developers, client developers), they
> can use MessageIdAdv if needed.
>
> Thanks,
> Penghui
>
> On Mon, Dec 19, 2022 at 11:45 AM Yunze Xu <y...@streamnative.io.invalid>
> wrote:
>
> > Hi Penghui,
> >
> > > It looks like LedgerHandleAdv to LedgerHandle in the bookkeeper.
> >
> > It's different. IMO, LedgerHandleAdv to LedgerHandle, or
> > MetadataStoreExtended to MetadataStoreExtend, are both added just to
> > avoid adding non-default methods to the original interface. But the
> > PulsarApiMessageId here has a more specific meaning that it represents
> > the proto.MessageIdData. I've thought about using the name
> > `MessageIdData`, but it could make some code complicated because both
> > these two `MessageIdData` could be used in the same file, which means
> > we have to write the very long type package name when using one of
> > them.
> >
> > > Why do we need to deprecate the BatchMessageAcker?
> >
> > Sorry I didn't make it clear. It's because if we still uses a
> > BatchMessageAcker in BatchMessageIdImpl, we still needs to use type
> > cast to call getAcker():
> >
> > ```java
> > if (msgId instanceof BatchMessageIdImpl) {
> > ((BatchMessageIdImpl) msgId).getAcker().ackIndividual();
> > }
> > ```
> >
> > Actually this proposal adds a new method to represent the ack set:
> >
> > ```java
> > default @Nullable BitSet getAckSet() {
> > return null;
> > }
> > ```
> >
> > We can replace the BatchMessageAcker with the BitSet and the batch
> > size. The introduction of the BatchMessageAcker also makes code more
> > complicated, e.g. the `getOriginalBatchSize` and `getBatchSize`
> > methods in `BatchMessageIdImpl`.
> >
> > Thanks,
> > Yunze
> >
> > On Mon, Dec 19, 2022 at 9:35 AM PengHui Li <peng...@apache.org> wrote:
> > >
> > > 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
> > > >
> >
