I didn't look into these two methods at the moment. But I think it's possible to
retain only the `fromByteArray`.
Thanks,
Yunze
On Tue, Nov 8, 2022 at 7:02 PM Enrico Olivelli <eolive...@gmail.com> wrote:
>
> Il giorno mar 8 nov 2022 alle ore 11:52 Yunze Xu
> <y...@streamnative.io.invalid> ha scritto:
> >
> > Hi Enrico,
> >
> > > We also need a way to represent this as a String or a byte[]
> >
> > We already have the `toByteArray` method, right?
>
> Yes, correct. So we are fine. I forgot about it and I answered too quickly.
>
> I am not sure if this can be in the scope of this initiative, but we
> should somehow get rid of
> stuff like "fromByteArrayWithTopic" vs "fromByteArray".
>
> Thanks
> Enrico
>
> >
> > Thanks,
> > Yunze
> >
> > On Tue, Nov 8, 2022 at 6:43 PM Enrico Olivelli <eolive...@gmail.com> wrote:
> > >
> > > Il giorno mar 8 nov 2022 alle ore 11:25 Yunze Xu
> > > <y...@streamnative.io.invalid> ha scritto:
> > > >
> > > > Hi all,
> > > >
> > > > Currently we have the following 5 implementations of MessageId:
> > > >
> > > > - MessageIdImpl: (ledger id, entry id, partition index)
> > > > - BatchMessageIdImpl: adds (batch index, batch size, acker), where
> > > > acker is a wrapper of a BitSet.
> > > > - ChunkMessageIdImpl: adds another MessageIdImpl that represents
> > > > the first MessageIdImpl of a BitSet.
> > > > - MultiMessageIdImpl: adds a map that maps the topic name to the
> > > > MessageId.
> > > > - TopicMessageIdImpl: adds the topic name and the partition name
> > > >
> > > > These implementations are such a mess. For example, when users get a
> > > > MessageId from `Producer#send`:
> > > >
> > > > ```java
> > > > var id = producer.send("msg");
> > > > ```
> > > >
> > > > There is no getter to get some specific fields like ledger id. You can
> > > > only see a representation from `toString` method and got some output
> > > > like "0:0:-1:0". Maybe this design is to hidden some details, but if
> > > > users don't know the details like ledger id and entry id, how could
> > > > you know what does "0:0:-1:0" mean? What if `MessageId#toString`'s
> > > > implementation changed? Should it be treated as a breaking change?
> > > >
> > > > The original definition of the underlying MessageIdData is much more
> > > > clear:
> > > >
> > > > ```proto
> > > > message MessageIdData {
> > > > required uint64 ledgerId = 1;
> > > > required uint64 entryId = 2;
> > > > optional int32 partition = 3 [default = -1];
> > > > optional int32 batch_index = 4 [default = -1];
> > > > repeated int64 ack_set = 5;
> > > > optional int32 batch_size = 6;
> > > >
> > > > // For the chunk message id, we need to specify the first chunk
> > > > message id.
> > > > optional MessageIdData first_chunk_message_id = 7;
> > > > }
> > > > ```
> > > >
> > > > IMO, MessageId should be a wrapper of MessageIdData. It's more natural
> > > > to have an interface like:
> > > >
> > > > ```java
> > > > interface MessageId {
> > > > long ledgerId();
> > > > long entryId();
> > > > Optional<Integer> partition();
> > > > Optional<Integer> batchIndex();
> > > > // ...
> > > > ```
> > >
> > > This is very good for client applications.
> > > We also need a way to represent this as a String or a byte[], this way
> > > client applications have a standard way to store
> > > message offsets into an external system (for instance when you want to
> > > user the Reader API and keep track of the position by yourself)
> > >
> > > Enrico
> > >
> > > >
> > > > Additionally, there are many places that use only the triple of
> > > > (ledger id, entry id, batch index) as the key to represent the position.
> > > > Currently, they are done by adding a conversion from
> > > > BatchMessageIdImpl to MessageIdImpl. However, it's more intuitive to
> > > > write something like:
> > > >
> > > > ```java
> > > > class MessageIdPosition implements Comparable<MessageIdPosition> {
> > > > private final MessageId messageId;
> > > > // TODO: compare only the triple (ledger, entry, batch)
> > > > ```
> > > >
> > > > Therefore, I'm going to write a proposal to redesign the MessageId
> > > > interface only by adding some getters. Regarding the 5 existing
> > > > implementations, I think we can drop them because they are a part
> > > > of `pulsar-client`, not `pulsar-client-api`.
> > > >
> > > > Please feel free to share your points.
> > > >
> > > > Thanks,
> > > > Yunze
