Sorry for the late reply due to recent work overload. 3. If the ByteBufferSerializer#serializeToByteBuffer(String, ByteBuffer) method does not maintain the same behavior as the ByteBufferSerializer#serialize(String, ByteBuffer) method, it will break the code logic for those users who originally used ByteBufferSerializer to serialize messages. Please refer to PR for KIP-872: https://github.com/apache/kafka/pull/12685/files#diff-42d8f5166459ee28f201ff9cec0080fc7845544a0089ac9e8f3e16864cc1193eR1006 and https://github.com/apache/kafka/pull/12685/files#diff-42d8f5166459ee28f201ff9cec0080fc7845544a0089ac9e8f3e16864cc1193eR1015 for details.
4. Changes to the ByteBufferSerializer documentation should be made only after a final decision is reached on #3. 5. The same applies to #5, and we should wait for a final decision on #3. Best, ShunKang Divij Vaidya <divijvaidy...@gmail.com> 于2022年11月7日周一 21:12写道： > 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 > > >> >> > > >> > > >
