LinShunKang commented on PR #12683:
URL: https://github.com/apache/kafka/pull/12683#issuecomment-1263296958
> > I re-run the test case in trunk as well as in your branch locally with
IDE, and both passes, so I think it's due to jenkins machines compute resource
limit itself.
> > But I got a separate question for the fix itself: normally when a
bytebuffer was allocated first, it was in `write` mode, and hence we would
expect to first `flip` it before reading data in serialization. But what if a
caller `flip` it first before calling the serde, in order to make it in `read`
mode?
> > No test failures give us some confidence that probably no one is yet
calling it that way, but we have to make sure no one will use it by first
`flip` themselves as well. So I think it's worth adding a few comments on top
of the `ByteBufferSerializer` emphasizing what it exactly is trying to do (i.e.
if it would try to reset limit / position / mark), and that callers does not
need to make it `read` before calling serialize.
> > Could you add such docs into the class before merging it?
>
> I made some changes to `ByteBufferSerializer#serialize(String,
ByteBuffer)` to keep the `ByteBuffer` offsets unchanged.
>
> And, I also add docs for ByteBufferSerializer.
@guozhangwang
Sorry, I found that keep the `ByteBuffer` offsets unchanged will break the
test `SerializationTest#allSerdesShouldRoundtripInput()`.
For example:
```
ByteBuffer buffer = ...; // Allocate a ByteBuffer
ByteBufferDeserializer deserializer = ...;
ByteBufferSerializer serializer = ...;
assertEquals(buffer, deserializer.deserialize("topic",
serializer.serialize("topic", buffer));
```
There are three ways to allocate a `ByteBuffer`:
* A: ByteBuffer#wrap(byte[])
* B: ByteBuffer.allocate(int).put(byte[])
* C: ByteBuffer.allocateDirect(int).put(byte[])
For B and C, executing the above test case will throw exception, due to the
buffer in write-mode, and if `ByteBufferSerializer#serialize(String,
ByteBuffer)` does not flip the buffer then the buffer still in write-mode, but
`ByteBufferDeserializer#deserialize(String, byte[])` use
`ByteBuffer#wrap(byte[])` to return a read-mode buffer, so even though the data
is equals but the offsets were different.
So, I revert the code to previous version that flip the buffer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
