adixitconfluent commented on code in PR #17739:
URL: https://github.com/apache/kafka/pull/17739#discussion_r1840737488
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -1602,8 +1602,6 @@ protected void
updateFetchOffsetMetadata(Optional<LogOffsetMetadata> fetchOffset
protected Optional<LogOffsetMetadata> fetchOffsetMetadata() {
lock.readLock().lock();
try {
- if (findNextFetchOffset.get())
Review Comment:
hi @junrao, now that I think more about it, IIUC, considering the common
case when all fetched data is acquirable -
1. acknowledgements/acquisition lock timeout/ release of records on session
close are the only places where we set `findNextFetchOffset` to true
2. In all the 3 scenarios mentioned above, if there is a change to the
`endOffset`, we update the `endOffset` (thereby `fetchOffsetMetadata` is also
updated automatically with our changes)
Hence, I feel that the findNextFetchOffset shouldn't be considered when
dealing with the common case.
In the not common cases, when Log Start Offset is later than the fetch
offset and we need to archive records, then we set `findNextFetchOffset` to
True. But we have done the minBytes implementation only for the common cases
right now, hence i feel the current change is correct. Please correct me if I
am wrong.
cc - @apoorvmittal10
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
