Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-15602: revert KAFKA-4852 #14617

Merged
merged 4 commits into from Oct 30, 2023
Merged

KAFKA-15602: revert KAFKA-4852 #14617

merged 4 commits into from Oct 30, 2023

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented Oct 23, 2023

This PR reverts

Also added some JavaDocs to ByteBufferSerializer to explain the contract.

philipnee
philipnee reviewed Oct 23, 2023
View reviewed changes
clients/src/main/java/org/apache/kafka/common/serialization/ByteBufferSerializer.java Outdated
* serializer.serialize(topic, buffer); // Serialize buffer
* </pre>
* </blockquote>
* {@code ByteBufferSerializer} always rewinds to zero for serialization.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"rewinds the position to zero"

Should we also explicitly mention rewind() is used?

@mjsax @philipnee
Update clients/src/main/java/org/apache/kafka/common/serialization/By…
3a44d08 
…teBufferSerializer.java

Co-authored-by: Philip Nee <pnee@confluent.io>
philipnee
philipnee reviewed Oct 24, 2023
View reviewed changes
Copy link
Collaborator

@philipnee philipnee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need to cover the test case for:
ByteBuffer.allocate(8).put("test".getBytes(UTF_8))
and
ByteBuffer.allocate(8).put("test".getBytes(UTF_8)).flip()

@mjsax
Copy link
Member Author

mjsax commented Oct 26, 2023

I would focusing on reverting -- if we want to update tests, we can do it in a separate PR?

@mjsax
Copy link
Member Author

mjsax commented Oct 26, 2023

@showuon Considering this for 3.5.2 bug-fix release. (Given that we wait for RocksDB testing to complete -- we are on it -- we could squeeze this one in, too?).

Also \cc @guozhangwang who merge the original PR.

@philipnee
Copy link
Collaborator

Hi @mjsax - I think so too.

@guozhangwang
Copy link
Contributor

Thanks @mjsax I read the JIRA description and all discussions and I agree we can revert the changes asap before we have a thorough KIP discussion on what should be the right semantics (and for that semantics I agree with your comment that is the only one that makes sense in the JIRA as well, btw).

@mjsax mjsax merged commit 4d04711 into apache:trunk Oct 30, 2023
1 check failed
@mjsax mjsax deleted the k4852-revert branch October 30, 2023 20:16
mjsax added a commit that referenced this pull request Oct 30, 2023
@mjsax
KAFKA-15602: revert KAFKA-4852 (#14617)
1d6d157 
This PR reverts
 - 51dbd17
 - 496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request Oct 30, 2023
@mjsax
KAFKA-15602: revert KAFKA-4852 (#14617)
86d4022 
This PR reverts
 - 51dbd17
 - 496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>
mjsax added a commit that referenced this pull request Oct 30, 2023
@mjsax
KAFKA-15602: revert KAFKA-4852 (#14617)
0c637ba 
This PR reverts
 - 51dbd17
 - 496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>
@mjsax
Copy link
Member Author

mjsax commented Nov 2, 2023

Merged to trunk and cherry-picked to 3.6, 3.5, and 3.4 branches.

mjsax added a commit to confluentinc/kafka that referenced this pull request Nov 22, 2023
@mjsax
KAFKA-15602: revert KAFKA-4852 (apache#14617)
1c541a8 
This PR reverts
 - apache@51dbd17
 - apache@496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>
@LinShunKang LinShunKang mentioned this pull request Feb 3, 2024
Open
3 tasks
anurag-harness pushed a commit to anurag-harness/kafka that referenced this pull request Feb 9, 2024
@mjsax @anurag-harness
KAFKA-15602: revert KAFKA-4852 (apache#14617)
51159f3 
This PR reverts
 - apache@51dbd17
 - apache@496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>
anurag-harness added a commit to anurag-harness/kafka that referenced this pull request Feb 9, 2024
@anurag-harness @mjsax
KAFKA-15602: revert KAFKA-4852 (apache#14617) (#182)
3c65ab4 
This PR reverts
 - apache@51dbd17
 - apache@496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>

Co-authored-by: Matthias J. Sax <matthias@confluent.io>
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
@mjsax @yyu1993
KAFKA-15602: revert KAFKA-4852 (apache#14617)
8187abe 
This PR reverts
 - apache@51dbd17
 - apache@496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>
tinaselenge pushed a commit to tinaselenge/kafka that referenced this pull request Mar 13, 2024
@mjsax @tinaselenge
KAFKA-15602: revert KAFKA-4852 (apache#14617)
2338d58 
This PR reverts
 - apache@51dbd17
 - apache@496ae05

Reviewers:  Philip Nee <pnee@confluent.io>, Guozhang Wang <guozhang@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipnee philipnee philipnee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mjsax @philipnee @guozhangwang