Hi, Yunze > 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.
I think `MessageIdAdv` and adding `topic name` is more confusing for non-partition-topic and redundant. If we don't couple TopicName and MessageID together, It is better to change our interface design into two parameters (topic name, MessageId). This is clearer and more intuitive, and it is easier for users to understand MessageId. MessageId also becomes simpler. I will sync this point to PIP-224-DISCUSS [0]. I don't have any problem with this pip now, thanks for your reply. +1 (non-binding) Thanks, Bo [0] https://lists.apache.org/thread/jhqy65cdyxzmmxnfsjm8rv9pbk76noxy Yunze Xu <y...@streamnative.io.invalid> 于2022年12月20日周二 15:27写道： > > 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
