apoorvmittal10 commented on code in PR #18696:
URL: https://github.com/apache/kafka/pull/18696#discussion_r1935621787
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -1847,19 +1948,30 @@ private boolean isRecordStateAcknowledged(RecordState
recordState) {
return recordState == RecordState.ACKNOWLEDGED || recordState ==
RecordState.ARCHIVED;
}
- private long findLastOffsetAcknowledged() {
- lock.readLock().lock();
+ // Visible for testing
+ long findLastOffsetAcknowledged() {
long lastOffsetAcknowledged = -1;
+ lock.readLock().lock();
try {
for (NavigableMap.Entry<Long, InFlightBatch> entry :
cachedState.entrySet()) {
InFlightBatch inFlightBatch = entry.getValue();
if (inFlightBatch.offsetState() == null) {
if
(!isRecordStateAcknowledged(inFlightBatch.batchState())) {
return lastOffsetAcknowledged;
}
+ // If initialReadGapOffset.gapStartOffset is less than or
equal to the last offset of the batch
+ // then we cannot identify the current inFlightBatch as
acknowledged. All the offsets between
+ // initialReadGapOffset.gapStartOffset and
initialReadGapOffset.endOffset should always be present
+ // in the cachedState
+ if (isInitialReadGapOffsetWindowActive() &&
inFlightBatch.lastOffset() >= initialReadGapOffset.gapStartOffset()) {
+ return lastOffsetAcknowledged;
+ }
lastOffsetAcknowledged = inFlightBatch.lastOffset();
} else {
for (Map.Entry<Long, InFlightState> offsetState :
inFlightBatch.offsetState.entrySet()) {
+ if (isInitialReadGapOffsetWindowActive() &&
offsetState.getKey() >= initialReadGapOffset.gapStartOffset()) {
+ return lastOffsetAcknowledged;
+ }
Review Comment:
Why do we require to check for gap offset inside a inflight batch?
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -1825,8 +1921,8 @@ private boolean canMoveStartOffset() {
NavigableMap.Entry<Long, InFlightBatch> entry =
cachedState.floorEntry(startOffset);
if (entry == null) {
- log.error("The start offset: {} is not found in the cached state
for share partition: {}-{}."
- + " Cannot move the start offset.", startOffset, groupId,
topicIdPartition);
+ log.info("The start offset: {} is not found in the cached state
for share partition: {}-{} " +
+ "as there is an acquirable gap at the beginning. Cannot move
the start offset.", startOffset, groupId, topicIdPartition);
return false;
Review Comment:
Can you help with the scenarios here again please.
##########
core/src/main/java/kafka/server/share/SharePartition.java:
##########
@@ -2160,6 +2234,43 @@ private long startOffsetDuringInitialization(long
partitionDataStartOffset) thro
}
}
+ // Visible for testing
+ long findLastOffsetAcknowledged() {
+ lock.readLock().lock();
+ long lastOffsetAcknowledged = -1;
+ try {
+ for (NavigableMap.Entry<Long, InFlightBatch> entry :
cachedState.entrySet()) {
+ InFlightBatch inFlightBatch = entry.getValue();
+ if (inFlightBatch.offsetState() == null) {
+ if
(!isRecordStateAcknowledged(inFlightBatch.batchState())) {
+ return lastOffsetAcknowledged;
+ }
+ // If initialReadGapOffset.gapStartOffset is less than or
equal to the last offset of the batch
+ // then we cannot identify the current inFlightBatch as
acknowledged. All the offsets between
+ // initialReadGapOffset.gapStartOffset and
initialReadGapOffset.endOffset should always be present
+ // in the cachedState
+ if (initialReadGapOffset != null &&
inFlightBatch.lastOffset() >= initialReadGapOffset.gapStartOffset()) {
+ return lastOffsetAcknowledged;
+ }
Review Comment:
Sure, then shouldn't this check happen outside if/else block?
--
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
