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