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.

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

KAFKA-18445: Remove LazyDownConversionRecords and LazyDownConversionRecordsSend #18445

Merged
merged 2 commits into from
Jan 9, 2025

Conversation

xijiu
Copy link
Contributor

@xijiu xijiu commented Jan 8, 2025

As title.

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added triage PRs from the community clients labels Jan 8, 2025
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@xijiu thanks for this patch!

@@ -203,8 +203,7 @@ public static FetchResponseData.PartitionData partitionResponse(int partition, E
* Returns `partition.records` as `Records` (instead of `BaseRecords`). If `records` is `null`, returns `MemoryRecords.EMPTY`.
*
* If this response was deserialized after a fetch, this method should never fail. An example where this would
* fail is a down-converted response (e.g. LazyDownConversionRecords) on the broker (before it's serialized and
* sent on the wire).
* fail is a down-converted response on the broker (before it's serialized and sent on the wire).
Copy link
Member

Choose a reason for hiding this comment

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

the message conversion has been removed by #18218, so this comment is stale. could you please remove it?

@@ -124,8 +124,7 @@ public static ShareFetchResponse parse(ByteBuffer buffer, short version) {
* Returns `partition.records` as `Records` (instead of `BaseRecords`). If `records` is `null`, returns `MemoryRecords.EMPTY`.
*
* <p>If this response was deserialized after a share fetch, this method should never fail. An example where this would
* fail is a down-converted response (e.g. LazyDownConversionRecords) on the broker (before it's serialized and
* sent on the wire).
* fail is a down-converted response on the broker (before it's serialized and sent on the wire).
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -75,6 +75,8 @@ <h5><a id="upgrade_400_notable" href="#upgrade_400_notable">Notable changes in 4
</li>
<li>The function <code>onNewBatch</code> in <code>org.apache.kafka.clients.producer.Partitioner</code> class was removed.
</li>
<li>The <code>org.apache.kafka.common.record.LazyDownConversionRecords</code> class and <code>org.apache.kafka.common.record.LazyDownConversionRecordsSend</code> class were removed.
Copy link
Member

Choose a reason for hiding this comment

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

they are not public APIs so we don't need to add them into upgrade.html

// of temporary memory used for down-conversion, etc. Pull out any such statistics from the underlying send
// and fold it up appropriately.
if (completedSend instanceof LazyDownConversionRecordsSend) {
if (recordConversionStats == null)
Copy link
Member

Choose a reason for hiding this comment

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

we can remove recordConversionStats and recordConversionStats(), right?

@github-actions github-actions bot removed the triage PRs from the community label Jan 9, 2025
@chia7712 chia7712 merged commit fcd98da into apache:trunk Jan 9, 2025
9 checks passed
chia7712 pushed a commit that referenced this pull request Jan 9, 2025
…cordsSend (#18445)

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants