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-18932: Removed usage of partition max bytes from share fetch requests #19148

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from
Open

KAFKA-18932: Removed usage of partition max bytes from share fetch requests #19148

wants to merge 11 commits into from
+581 −962

Conversation

adixitconfluent
Copy link
Contributor

@adixitconfluent adixitconfluent commented Mar 7, 2025 •
edited
Loading

What

This PR aims to remove the usage of partition max bytes from share fetch requests. Partition Max Bytes is being defined by PartitionMaxBytesStrategy which was added to the broker as part of PR #17870

Testing

The code has been tested with the help of already present unit and integration tests.

@github-actions github-actions bot added triage PRs from the community core Kafka Broker KIP-932 Queues for Kafka clients labels Mar 7, 2025
@AndrewJSchofield AndrewJSchofield added ci-approved and removed triage PRs from the community labels Mar 7, 2025
@adixitconfluent adixitconfluent marked this pull request as ready for review March 7, 2025 11:56
apoorvmittal10
apoorvmittal10 reviewed Mar 7, 2025
View reviewed changes
Copy link
Collaborator

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. One high level query before I proceed for review.

apoorvmittal10
apoorvmittal10 reviewed Mar 9, 2025
View reviewed changes
Copy link
Collaborator

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Some comments.

AndrewJSchofield
AndrewJSchofield requested changes Mar 10, 2025
View reviewed changes
Copy link
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It's good to see some of the cruft being removed.

@AndrewJSchofield
Copy link
Member

Please update clients/src/main/resources/common/message/ShareFetchRequest.json too to remove PartitionMaxBytes.

@adixitconfluent
Copy link
Contributor Author

Please update clients/src/main/resources/common/message/ShareFetchRequest.json too to remove PartitionMaxBytes.

done this in my latest commit, it has triggered a whole bunch of changes :)

@adixitconfluent adixitconfluent changed the title KAFKA-18932: Removed usage of partition max bytes from share fetch requests on the broker KAFKA-18932: Removed usage of partition max bytes from share fetch requests Mar 11, 2025
apoorvmittal10
apoorvmittal10 reviewed Mar 11, 2025
View reviewed changes
Copy link
Collaborator

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

LGTM, left some comments.

@github-actions github-actions bot added tools connect kraft mirror-maker-2 storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature build Gradle build or GitHub Actions generator RPC and Record code generator labels Mar 12, 2025
@adixitconfluent
Copy link
Contributor Author

@apoorvmittal10 , I have addressed your comments. Please review when you can. Also, I had to do a force push to fix the branch history because it got corrupted by a merge commit performed by IntelliJ. That's why you see so many labels in the PR. If possible, can you remove the extra labels streams, producer, tools, connect, kraft, mirror-maker-2, storage, tiered-storage, build and generator?
Thanks!

@AndrewJSchofield AndrewJSchofield removed streams producer tools connect kraft mirror-maker-2 storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature generator RPC and Record code generator labels Mar 12, 2025
@apoorvmittal10 apoorvmittal10 added generator RPC and Record code generator and removed consumer labels Mar 12, 2025
apoorvmittal10
apoorvmittal10 reviewed Mar 12, 2025
View reviewed changes
Copy link
Collaborator

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comments and a question.

apoorvmittal10
apoorvmittal10 approved these changes Mar 12, 2025
View reviewed changes
Copy link
Collaborator

@apoorvmittal10 apoorvmittal10 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoorvmittal10 apoorvmittal10 apoorvmittal10 approved these changes

@AndrewJSchofield AndrewJSchofield AndrewJSchofield approved these changes

Assignees
No one assigned
Labels
build Gradle build or GitHub Actions ci-approved clients consumer core Kafka Broker generator RPC and Record code generator KIP-932 Queues for Kafka
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adixitconfluent @AndrewJSchofield @apoorvmittal10