I think the topic name will not be transmitted to the broker. The client side used the class generated by the protobuf message. Or, we can create another class to avoid coupling issues, but it will introduce more changes and copy data from one structure to another. For the long-term, I think it should be a good way if we don't have blockers with this solution. Because I don't think there is a higher priority in the long run than keeping the protocol clear.
If the above options are not feasible. At least, we should clarify it in the proposal and add comments in the proto file to avoid other clients transmitting the topic name to the broker. Thanks, Penghui On Fri, May 12, 2023 at 5:59 PM Asaf Mesika <asaf.mes...@gmail.com> wrote: > I don't get it - you say msgId is a data structure contained within > MessageId implementation, right? I presume msgId is the data structure the > client transmit to the server, so that means you are transmitting topic to > the server? > > > On Fri, May 12, 2023 at 7:45 AM Rajan Dhabalia <rdhaba...@apache.org> > wrote: > > > Thank you for sharing your knowledge about the PIP which should be > created > > before PR and I think everyone in the community knows about it. but you > can > > check the PR for context which was blocked for sometime and we decided to > > create PIP with proto changes. > > > > This PIP/PR tries to fix the issue where partitioned topic fails while > > acking deserialized messageId. topic name will be part of MsgIdData which > > is the data-structure used by messageID to store msgID context along with > > partition, batching, and other metadata. topic name will be attached only > > when the user tries to serialize and deserialize the messageId which will > > be purely client side implementation and in other cases it will not be > > transmitted to server. Also, partitioned topic's abstract concept for > user > > and messageID must be also remain abstract for users and users must not > > know about different implementation of messageId and our goal is to > > maintain that abstraction without telling user about MessageIdImpl or > > TopicMessageIdImpl. > > > > Thanks, > > Rajan > > > > > > On Thu, May 11, 2023 at 7:29 AM Asaf Mesika <asaf.mes...@gmail.com> > wrote: > > > > > Hi Rajan, > > > > > > A few comments on the PIP as I couldn't understand it fully as some > > pieces > > > of information is missing. > > > > > > First, I would like to remind about the rules, that exists in the > > beginning > > > of the PIP template: > > > > > > <!-- > > > RULES > > > * Never place a link to an external site like Google Doc. The proposal > > > should be in this issue entirely. > > > * Use a spelling and grammar checker tools if available for you (there > > are > > > plenty of free ones) > > > > > > PROPOSAL HEALTH CHECK > > > I can read the design document and understand the problem statement and > > > what you plan to change *without* resorting to a couple of hours of > code > > > reading just to start having a high level understanding of the change. > > > --> > > > > > > > > > In this specific case > > > 1. I would include explanation and detail the data structures fields of > > > objects you mentioned, such as: MessageIdImpl and MessageIdData. > > > 2. I would not put a PR as the design section, so I need to read code > to > > > understand what the exact solution details. > > > > > > You wrote: > > > > > > > Pulsar api provides MessageId > > > > < > > > > > > https://github.com/apache/pulsar/blob/master/pulsar-client-api/src/main/java/org/apache/pulsar/client/api/MessageId.java > > > > > > interface > > > > which is generally used by producer and consumer applications to > manage > > > > topic offset. > > > > > > > > > I think it's used to allow consumers to acknowledge (can be per > message) > > so > > > offset if wrong terminology here. > > > For producers, not sure exactly its usage. Maybe if they need to refer > to > > > this message later when reading by Reader interface. > > > I would correct this section. > > > > > > However, right now Pulsar doesn't support correct deserialization of > > > > multi-topic or partitioned-topic because of that 1acknowledge` API > call > > > > fails for those topics with below error > > > > > > > > > You're saying that the acknowledgement API method signature receives > > > MessageId, but do not receive TopicMessageId? > > > > > > I have a few questions on that: > > > > > > 1. The acknowledgement API is part of Pulsar binary protocol. Is your > > plan > > > to alter that protocol so it will also include the topic field as part > of > > > the message ID? > > > > > > 2. I think your PIP needs to explain the following items which are > > missing > > > as context: > > > - There are two implementation for MessageId interface, one for topic > and > > > one for partitioned topic. > > > - The problem is that the serialization/desrialization method is used > > > mainly for translating the ID into the binary protocol, which only > > requires > > > the ID (ledger ID, entry ID). > > > - The reason for that is that once you created a consumer, it has a > topic > > > attached to it. Transferring the topic for the ack is redundant. > > > > > > All of this needs to be in the background. > > > > > > I have several ideas on solving that, which IMO should mainly be in the > > > client level, but I must get answers to the questions above before I > can > > > continue. > > > > > > Last note > > > You have basically placed a link to a pull request as the design > solution > > > (high-level/detailed design). > > > The whole idea of the design is that you describe the solution without > > > resorting to code. > > > IMO you should amend the design, state the goal shortly, and have a > high > > > level design section which contains 1-2 short paragraphs describing > > exactly > > > your solution. > > > > > > Thanks, > > > > > > Asaf > > > > > > > > > > > > On Tue, May 9, 2023 at 3:24 PM Yunze Xu <x...@apache.org> wrote: > > > > > > > I'm talking about whether to add a new separate API. I'm concerned > > > > about whether existing applications would be affected, no matter if > > > > the existing implementation has the limitation. If yes, we should > > > > document it in the PIP so that users can know that. > > > > > > > > > it's a new optional field which would not break the compatibility > > > > > > > > And yes, I just confirmed it with simple demos in my local env. So > I'm > > > > +1 to this proposal. > > > > > > > > Thanks, > > > > Yunze > > > > > > > > On Tue, May 9, 2023 at 3:05 PM Rajan Dhabalia <rdhaba...@apache.org> > > > > wrote: > > > > > > > > > > Weill there are multiple things: it's a new optional field which > > would > > > > not > > > > > break the compatibility , also current messaegId serialization and > > > > > deserialization anyway only impact multi-topic consumer which is > > > already > > > > > broken or has the limitation and, adding a new separate API for > > > > partitioned > > > > > topic is not only not acceptable but creates too much confusion for > > > users > > > > > to use separate ack APIs for non-partition and partition topics and > > > that > > > > > also breaks partitioned topic abstraction which we would like to > > avoid. > > > > > > > > > > Thanks, > > > > > Rajan > > > > > > > > > > On Mon, May 8, 2023 at 11:27 PM Yunze Xu <x...@apache.org> wrote: > > > > > > > > > > > It seems that `TopicMessageIdImpl#toByteArray` now could > serialize > > > the > > > > > > optional topic field to the bytes. I didn't test it now but I > have > > a > > > > > > concern about whether it would bring a breaking change. > > > > > > > > > > > > Assuming there are two applications (let's say A and B) based on > an > > > > > > older Pulsar client, A writes serialized bytes into a file, B > reads > > > > > > bytes from the file and parses it to a MessageId. If A upgraded > its > > > > > > Pulsar client to the latest while B did not, what would happen? > > Could > > > > > > B still get the correct MessageId or the bytes would not be able > to > > > > > > parsed? > > > > > > > > > > > > P.S. it's better to add the API changes and potential breaking > > > changes > > > > > > in the proposal. > > > > > > > > > > > > Thanks, > > > > > > Yunze > > > > > > > > > > > > On Tue, May 9, 2023 at 1:59 PM Rajan Dhabalia < > > rdhaba...@apache.org> > > > > > > wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > Pulsar api provides MessageId interface which is generally used > > by > > > > > > producer > > > > > > > and consumer applications to manage topic offset. Sometimes, > > these > > > > > > > applications would like to serialize and deserialize > messageIds, > > > > > > > specifically consumer app which would like to persist messageId > > and > > > > ack > > > > > > > with those messageIds by deserializing them. However, right now > > > > Pulsar > > > > > > > doesn't support correct deserialization of multi-topic or > > > > > > partitioned-topic > > > > > > > because of that 1acknowledge` API call fails for those topics > > with > > > > below > > > > > > > error: > > > > > > > "Only TopicMessageId is allowed to acknowledge for a > multi-topics > > > > > > consumer" > > > > > > > > > > > > > > MessageIdImpl stores id metadata into MessageIdData which > doesn't > > > > contain > > > > > > > context about topic name to find out which topic belongs to > this > > > > > > MessageID. > > > > > > > Therefore, we need to add topic-name into MessageIdData and > allow > > > > > > > multi-topic/partitioned topics to deserialize messages > correctly > > > so, > > > > > > > consumer app can perform as expected. > > > > > > > > > > > > > > Please visit PIP for any suggestions: > > > > > > > https://github.com/apache/pulsar/issues/20221 > > > > > > > > > > > > > > This PIP is created with PR: > > > > https://github.com/apache/pulsar/pull/19944 > > > > > > > > > > > > > > Thanks, > > > > > > > Rajan > > > > > > > > > > > > > > > >
