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
> > >
>
