Hi Bo,
> But when `topicName` is not in the `PulsarApiMessageId`, they should use
> `messageId instanceof TopicMessageId` to get the `topicName`
The scenario only happens for `acknowledge` APIs because I don't want
to add more overloads in PIP-224. The new `seek` overloads in PIP-224
just accept the `TopicMessageId` parameter.
Combining the PIP-224 and PIP-229, the code will look like:
```java
// MultiTopicsConsumerImpl
void acknowledge(MessageId msgId) {
if (!(msgId instanceof TopicMessageId)) {
throw new PulsarClientException.NotAllowedException("");
}
consumers.get(((TopicMessageId) msgId).getOwnerTopic()).acknowledge(msgId);
}
void seek(TopicMessageId topicMessageId) {
consumers.get(topicMessageId.getOwnerTopic()).seek(topicMessageId);
}
```
```java
// ConsumerImpl
void acknowledge(MessageId msgId) {
internalAcknowledge((MessageIdAdv) msgId); // we never need the
owner topic now
}
void seek(TopicMessageId topicMessageId) {
internalSeek((MessageIdAdv) topicMessageId);
}
```
That's why we need two interfaces. The use cases of TopicMessageId are
very rare and limited. This interface is introduced mainly because we
don't have a String parameter in seek and acknowledge before. It's a
tradeoff between compatibility and complexity. But the use cases of
MessageIdAdv are very common.
Thanks,
Yunze
On Mon, Dec 19, 2022 at 9:00 PM 丛搏 <bog...@apache.org> wrote:
>
> Hi Yunze :
>
> I have no reason to must add the `getTopicName()` into the
> `PulsarApiMessageId` (or named `MessageIdAdv `).
> I just thought of a scenario that users would use. For the consumer
> side, they don't know whether the topic is a partitioned topic or a
> non-partitioned topic. Maybe they don't care about it. But when
> `topicName` is not in the `PulsarApiMessageId`, they should use
> `messageId instanceof TopicMessageId` to get the `topicName`, if this
> `topicName` is in the `PulsarApiMessageId`, they only use the unified
> logic to get the field from the messageId `PulsarApiMessageId
> pulsarApiMessageId = (PulsarApiMessageId) messageId`
>
> These are just some of my views, and it will not block this PIP.
>
> Thanks,
> Bo
>
> Yunze Xu <y...@streamnative.io.invalid> 于2022年12月19日周一 10:41写道：
> >
> > Hi Bo,
> >
> > For a single-topic consumer, the MessageId implementations returned by
> > receive() should be a MessageIdImpl or BatchedMessageIdImpl. Is there
> > any reason to add a `getTopicName()` method for them (via extending
> > TopicMessageId)? If yes, we have to use the default implementation of
> > `getTopicName()` that returns null without modifying any existing
> > code. If you don't want to return a null value, you have to add new
> > logic that sets the topic explicitly when creating the received
> > message.
> >
> > Thanks,
> > Yunze
> >
> > On Sun, Dec 18, 2022 at 3:44 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
