Skip to content

Account for offset overlaps in RemoteLog deletion by size#34

Merged
satishd merged 4 commits into
satishd:2.8.x-tiered-storagefrom
divijvaidya:uber-27
Jul 18, 2022
Merged

Account for offset overlaps in RemoteLog deletion by size#34
satishd merged 4 commits into
satishd:2.8.x-tiered-storagefrom
divijvaidya:uber-27

Conversation

@divijvaidya

@divijvaidya divijvaidya commented May 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Resolves #27

This fixes a bug in calculation of size when retention by size is enabled for a topic with remote storage enabled.

@divijvaidya

divijvaidya commented May 12, 2022

Copy link
Copy Markdown
Collaborator Author

@kamalcph @satishd @wyuka please review.

@divijvaidya divijvaidya linked an issue May 12, 2022 that may be closed by this pull request
Ensure the log segments stored on both the remote and local storages are not counted twice when comparing the size of the log to the retention size setting #27
Closed
kamalcph
kamalcph reviewed May 19, 2022
View reviewed changes

@kamalcph kamalcph left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, @divijvaidya for the patch! Left some minor comments.

Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated
Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated
Comment thread core/src/test/scala/unit/kafka/log/remote/RemoteLogManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/log/remote/RemoteLogManagerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/log/remote/RemoteLogManagerTest.scala Outdated
kamalcph
kamalcph approved these changes Jun 15, 2022
View reviewed changes

@kamalcph kamalcph left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing this bug!

satishd
satishd reviewed Jun 28, 2022
View reviewed changes

@satishd satishd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks @divijvaidya for the PR. Left a few comments.

Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated
Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated
Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated
Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated
Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated

@satishd satishd Jun 27, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please change the log level as DEBUG or INFO because this is not an error or an unexpected scenario.

Comment thread core/src/main/scala/kafka/log/remote/RemoteLogManager.scala Outdated
@divijvaidya

divijvaidya commented Jul 12, 2022

Copy link
Copy Markdown
Collaborator Author

@satishd @kamalcph this is ready for your review. New tests have been added and some more fixes have been added which are associated with the same context of calculation of log size.

satishd
satishd approved these changes Jul 18, 2022
View reviewed changes

@satishd satishd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks @divijvaidya for addressing the review comments, LGTM.

@kamalcph

kamalcph commented Jul 18, 2022

Copy link
Copy Markdown
Collaborator

Thanks for fixing this issue! LGTM.

@satishd satishd merged commit 8a87a5c into satishd:2.8.x-tiered-storage Jul 18, 2022
@divijvaidya divijvaidya deleted the uber-27 branch July 18, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@satishd satishd satishd approved these changes

@kamalcph kamalcph kamalcph approved these changes

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

4 participants

@divijvaidya @kamalcph @satishd @clolov