Apologies for the late reply. I will be more proactive on this thead moving ahead.
3. Understood. Perhaps, `ByteBuffer#asReadOnlyBuffer()` is not the right solution and I acknowledge that the current API modifies the offsets of the user provided input when it calls flip(). I still believe that we should not be modifying (both offsets and data) the user provided input bytebuffer, but I understand that it would require a change in semantics wrt existing API behaviour. I would vote for having new/correct semantics introduced with this KIP itself (else, as John mentioned, we would have to add a new method later). I am advocating for new semantics because it clarifies the contract rigorously (immutable state of input params) which enables consumers of the API to do some nifty things on their end wrt memory management. 4. Regarding #4 I mentioned earlier, do you agree with the comment? If yes, can you please add the JavaDocs to the API contract defined in KIP? 5. From a backward compatibility perspective, would the offsets for the user provided ByteBuffer (key & value) remain the same as the earlier implementation for `doSend()`? Could we add a test to verify this? Perhaps, this is worth clarifying in the KIP? -- Divij Vaidya On Sun, Nov 6, 2022 at 4:23 PM John Roesler <vvcep...@apache.org> wrote: > Thanks for the reply, ShunKang! > > You’re absolutely right, we should not change the behavior of the existing > method. > > Regarding the new method, I was thinking that this is a good opportunity > to correct what seems to be strange semantics in the original one. If we > keep the same semantics and want to correct it later, we’ll be forced to > introduce yet another method later. This especially makes sense if we’re > thinking of deprecating the original method. But if you think it’s better > to keep it the way it is, I’m fine with it. > > I have no other comments. > > Thanks again for the KIP, > John > > On Sat, Nov 5, 2022, at 11:59, ShunKang Lin wrote: > > Hi John, > > > > Thanks for your comments! > > > > For your first question, I see some unit test cases that give us a > > ByteBuffer not set to read before calling > > `ByteBufferSerializer#serialize(String, ByteBuffer)`, e.g. > > `ArticleSerializer`, `AugmentedArticleSerializer`, > > `AugmentedCommentSerializer` and `CommentSerializer`. If we don't flip > the > > ByteBuffer inside the `ByteBufferSerializer#serialize(String, > ByteBuffer)` > > it will break user code using `ByteBufferSerializer#serialize(String, > > ByteBuffer)`, and if we don't flip the ByteBuffer inside > > the `ByteBufferSerializer#serializeToByteBuffer(String, ByteBuffer)`, it > > will be even more strange to the user, because > > `ByteBufferSerializer#serialize(String, ByteBuffer)` and > > `ByteBufferSerializer#serializeToByteBuffer(String, ByteBuffer)` require > > users use the ByteBufferSerializer in two different ways. So if we think > of > > `ByteBufferSerialize#serializeToByteBuffer(String, ByteBuffer)` as > setting > > up a ByteBuffer to read later, is it more acceptable? > > > > For your second question, I plan to ultimately replace byte[] with > > ByteBuffer, I will document the intent in your KIP and JavaDocs later. > > > > I will clarify that if a Serializer implements the new method, then the > old > > one will never be called. > > > > Best, > > ShunKang > > > > John Roesler <vvcep...@apache.org> 于2022年11月4日周五 22:42写道： > > > >> Hi ShunKang, > >> > >> Thanks for the KIP! > >> > >> I’ve been wanting to transition toward byte buffers for a while, so this > >> is a nice start. > >> > >> I thought it was a bit weird to flip the buffer inside the serializer, > but > >> I see the existing one already does that. I would have thought it would > >> make more sense for the caller to give us a buffer already set up for > >> reading. Do you think it makes sense to adopt this pattern for the new > >> method? > >> > >> Do you plan to keep the new methods as optional indefinitely, or do you > >> plan to ultimately replace byte[] with ByteBuffer? If it’s the latter, > then > >> it would be good to document the intent in your KIP and JavaDocs. > >> > >> It would be good to clarify that if a Serializer implements the new > >> method, then the old one will never be called. That way, implementations > >> can just throw an exception on that method instead of implementing both. > >> > >> Thanks again! > >> -John > >> > >> On Wed, Nov 2, 2022, at 20:14, ShunKang Lin wrote: > >> > Bump this thread again : ) > >> > > >> > ShunKang Lin <linshunkang....@gmail.com>于2022年9月25日 周日23:59写道： > >> > > >> >> Hi all, I'd like to start a new discussion thread on KIP-872 (Kafka > >> >> Client) which proposes that add Serializer#serializeToByteBuffer() to > >> >> reduce memory copying. > >> >> > >> >> KIP: > >> >> > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=228495828 > >> >> Thanks, ShunKang > >> >> > >> >
