Skip to content

KAFKA-14540: Fix DataOutputStreamWritable#writeByteBuffer#13032

Merged
viktorsomogyi merged 3 commits intoapache:trunkfrom
michaeljmarshall:fix-write-byte-buffer
Jan 10, 2023
Merged

KAFKA-14540: Fix DataOutputStreamWritable#writeByteBuffer#13032
viktorsomogyi merged 3 commits intoapache:trunkfrom
michaeljmarshall:fix-write-byte-buffer

Conversation

@michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Dec 21, 2022 •
edited
Loading

When writing a ByteBuffer backed by a HeapBuffer to a DataOutputStream, it is necessary to pass in the offset and the position, not just the position. It is also necessary to pass the remain length, not the limit. The current code results in writing the wrong data to DataOutputStream. While the DataOutputStreamWritable is used in the project, I do not see any references that would utilize this code path, so this bug fix is relatively minor.

I added a new test to cover the exact bug. The test fails without this change.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

showuon
showuon reviewed Dec 21, 2022
View reviewed changes
Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

@michaeljmarshall , thanks for the PR. Nice catch! Left some comments.

showuon
showuon approved these changes Dec 22, 2022
View reviewed changes
Copy link
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR!

viktorsomogyi
viktorsomogyi requested changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@viktorsomogyi viktorsomogyi left a comment

Choose a reason for hiding this comment

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

Realized that there are tests failures when running these with gradle (although weirly enough they were fine in intellij), please fix them.

@michaeljmarshall
Copy link
Member Author

michaeljmarshall commented Jan 6, 2023

Thanks for catching my mistake @viktorsomogyi. My mistake was trusting IntelliJ to run the test.

viktorsomogyi
viktorsomogyi approved these changes Jan 9, 2023
View reviewed changes
Copy link
Contributor

@viktorsomogyi viktorsomogyi left a comment

Choose a reason for hiding this comment

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

LGTM, I'll wait some time for others if they have any more comments and also the rebuild, after that I'll merge it.

@showuon
Copy link
Member

showuon commented Jan 10, 2023

Nice catch for assertArrayEquals issue. LGTM! Thanks.

@viktorsomogyi
Copy link
Contributor

viktorsomogyi commented Jan 10, 2023

Looks like there are 3 flaky test failures, I'll merge this one.

@viktorsomogyi viktorsomogyi merged commit def8d72 into apache:trunk Jan 10, 2023
ijuma added a commit to fvaleri/kafka that referenced this pull request Jan 13, 2023
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into move-fdi
bc80173 
* apache-github/trunk:
  KAFKA-14601: Improve exception handling in KafkaEventQueue apache#13089
  KAFKA-14367; Add `OffsetCommit` to the new `GroupCoordinator` interface (apache#12886)
  KAFKA-14530: Check state updater more often (apache#13017)
  KAFKA-14304 Use boolean for ZK migrating brokers in RPC/record (apache#13103)
  KAFKA-14003 Kafka Streams JUnit4 to JUnit5 migration part 2 (apache#12301)
  KAFKA-14607: Move Scheduler/KafkaScheduler to server-common (apache#13092)
  KAFKA-14367; Add `OffsetFetch` to the new `GroupCoordinator` interface (apache#12870)
  KAFKA-14557; Lock metadata log dir (apache#13058)
  MINOR: Implement toString method for TopicAssignment and PartitionAssignment (apache#13101)
  KAFKA-12558: Do not prematurely mutate internal partition state in Mirror Maker 2 (apache#11818)
  KAFKA-14540: Fix DataOutputStreamWritable#writeByteBuffer (apache#13032)
  KAFKA-14600: Reduce flakiness in ProducerIdExpirationTest (apache#13087)
  KAFKA-14279: Add 3.3.x streams system tests (apache#13077)
  MINOR: bump streams quickstart pom versions and add to list in gradle.properties (apache#13064)
  MINOR: Update KRaft cluster upgrade documentation for 3.4 (apache#13063)
  KAFKA-14493: Introduce Zk to KRaft migration state machine STUBs in KRaft controller. (apache#12998)
  KAFKA-14570: Fix parenthesis in verifyFullFetchResponsePartitions output (apache#13072)
  MINOR: Remove public mutable fields from ProducerAppendInfo (apache#13091)
ijuma added a commit to confluentinc/kafka that referenced this pull request Jan 17, 2023
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into confluentinc/…
68d17cc 
…master

* apache-github/trunk: (23 commits)
  MINOR: Include the inner exception stack trace when re-throwing an exception (apache#12229)
  MINOR: Fix docs to state that sendfile implemented in `TransferableRecords` instead of `MessageSet` (apache#13109)
  Update ProducerConfig.java (apache#13115)
  KAFKA-14618; Fix off by one error in snapshot id (apache#13108)
  KAFKA-13709 (follow-up): Avoid mention of 'exactly-once delivery' or 'delivery guarantees' in Connect (apache#13106)
  KAFKA-14367; Add `TxnOffsetCommit` to the new `GroupCoordinator` interface (apache#12901)
  KAFKA-14568: Move FetchDataInfo and related to storage module (apache#13085)
  KAFKA-14612: Make sure to write a new topics ConfigRecords to metadata log iff the topic is created (apache#13104)
  KAFKA-14601: Improve exception handling in KafkaEventQueue apache#13089
  KAFKA-14367; Add `OffsetCommit` to the new `GroupCoordinator` interface (apache#12886)
  KAFKA-14530: Check state updater more often (apache#13017)
  KAFKA-14304 Use boolean for ZK migrating brokers in RPC/record (apache#13103)
  KAFKA-14003 Kafka Streams JUnit4 to JUnit5 migration part 2 (apache#12301)
  KAFKA-14607: Move Scheduler/KafkaScheduler to server-common (apache#13092)
  KAFKA-14367; Add `OffsetFetch` to the new `GroupCoordinator` interface (apache#12870)
  KAFKA-14557; Lock metadata log dir (apache#13058)
  MINOR: Implement toString method for TopicAssignment and PartitionAssignment (apache#13101)
  KAFKA-12558: Do not prematurely mutate internal partition state in Mirror Maker 2 (apache#11818)
  KAFKA-14540: Fix DataOutputStreamWritable#writeByteBuffer (apache#13032)
  KAFKA-14600: Reduce flakiness in ProducerIdExpirationTest (apache#13087)
  ...
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
@michaeljmarshall @guozhangwang
KAFKA-14540: Fix DataOutputStreamWritable#writeByteBuffer (apache#13032)
34d4cf7 
When writing a ByteBuffer backed by a HeapBuffer to a DataOutputStream, it is necessary to pass in the offset and the position, not just the position. It is also necessary to pass the remain length, not the limit. The current code results in writing the wrong data to DataOutputStream. While the DataOutputStreamWritable is used in the project, I do not see any references that would utilize this code path, so this bug fix is relatively minor.

I added a new test to cover the exact bug. The test fails without this change.
@michaeljmarshall michaeljmarshall deleted the fix-write-byte-buffer branch February 24, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ijuma ijuma ijuma left review comments

@viktorsomogyi viktorsomogyi viktorsomogyi approved these changes

@showuon showuon showuon approved these changes

+2 more reviewers

@lhotari lhotari lhotari approved these changes

@eolivelli eolivelli eolivelli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@michaeljmarshall @showuon @viktorsomogyi @ijuma @lhotari @eolivelli