KAFKA-18022: fetchOffsetMetadata handling for minBytes estimation in both common/uncommon cases of share fetch #17825
Conversation
github-actions
bot
added
core
There was a problem hiding this comment.
@adixitconfluent : Thanks for the PR. Just a couple of minor comments.
| * FetchOffsetMetadata class is used to cache offset and its log metadata. | ||
| */ | ||
| static final class OffsetMetadata { | ||
| private long offset; |
There was a problem hiding this comment.
It's useful to add a comment that this offset could be different from offsetMetadata.messageOffset if it's in the middle of a batch.
| private LogOffsetMetadata offsetMetadata; | ||
|
|
||
| OffsetMetadata() { | ||
| } |
There was a problem hiding this comment.
Should we initialize the fields?
There was a problem hiding this comment.
I am not sure if initializing the fields has any benefits. The only value initialization inside the constructor that makes sense is offset=-1 i.e. an invalid case. We don't really need this initialization since we can use offsetMetadata() == null to check if the object fetchOffsetMetadata has been cached yet or not.
There was a problem hiding this comment.
Yes, since 0 is a valid offset, it's better to initialize offset to -1.
There was a problem hiding this comment.
i've pushed the change in my latest commit. Thanks!
|
Hi @junrao , I have pushed the suggested change. Please review this PR when you get a chance. Thanks! |
About
In reference to comments #17539 (comment) and #17739 (comment), I have updated the way we handle fetchOffsetMetadata update and get so that minBytes estimation is consistent for both common and uncommon cases of share fetch.
Testing
Testing has been done with the help of new/already present unit tests and already present integration tests.