You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/09/25 08:27:46 UTC
[GitHub] [kafka] LinShunKang opened a new pull request, #12683: KAFKA-4852: Fix ByteBufferSerializer#serialize(String, ByteBuffer) not compatible with offsets
LinShunKang opened a new pull request, #12683:
URL: https://github.com/apache/kafka/pull/12683
Currently ByteBufferSerializer#serialize(String, ByteBuffer) is not compatible with offsets, please check: https://issues.apache.org/jira/browse/KAFKA-4852
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] LinShunKang commented on pull request #12683: KAFKA-4852: Fix ByteBufferSerializer#serialize(String, ByteBuffer) not compatible with offsets
Posted by GitBox <gi...@apache.org>.
LinShunKang commented on PR #12683:
URL: https://github.com/apache/kafka/pull/12683#issuecomment-1258853602
> Re-starting tests, if they pass I think we can merge it.
I re-run the tests on branch trunk, but it also fails, and the branch fix/ByteBufferSerializer is checked out from branch-trunk.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] LinShunKang commented on pull request #12683: KAFKA-4852: Fix ByteBufferSerializer#serialize(String, ByteBuffer) not compatible with offsets
Posted by GitBox <gi...@apache.org>.
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] LinShunKang commented on pull request #12683: KAFKA-4852: Fix ByteBufferSerializer#serialize(String, ByteBuffer) not compatible with offsets
Posted by GitBox <gi...@apache.org>.
LinShunKang commented on PR #12683:
URL: https://github.com/apache/kafka/pull/12683#issuecomment-1262595051
> 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.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #12683: KAFKA-4852: Fix ByteBufferSerializer#serialize(String, ByteBuffer) not compatible with offsets
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12683:
URL: https://github.com/apache/kafka/pull/12683#issuecomment-1258711127
Re-starting tests, if they pass I think we can merge it.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang merged pull request #12683: KAFKA-4852: Fix ByteBufferSerializer#serialize(String, ByteBuffer) not compatible with offsets
Posted by GitBox <gi...@apache.org>.
guozhangwang merged PR #12683:
URL: https://github.com/apache/kafka/pull/12683
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #12683: KAFKA-4852: Fix ByteBufferSerializer#serialize(String, ByteBuffer) not compatible with offsets
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12683:
URL: https://github.com/apache/kafka/pull/12683#issuecomment-1261408099
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?
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?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org