junrao commented on code in PR #15476:
URL: https://github.com/apache/kafka/pull/15476#discussion_r1522196043
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogValidator.java:
##########
@@ -293,11 +293,11 @@ public ValidationResult
assignOffsetsNonCompressed(LongRef offsetCounter,
if (timestampType == TimestampType.LOG_APPEND_TIME) {
maxTimestamp = now;
- offsetOfMaxTimestamp = initialOffset;
- }
-
- if (toMagic >= RecordBatch.MAGIC_VALUE_V2) {
- offsetOfMaxTimestamp = offsetCounter.value - 1;
+ if (toMagic >= RecordBatch.MAGIC_VALUE_V2) {
+ offsetOfMaxTimestamp = offsetCounter.value - 1;
Review Comment:
@johnnychhsu : Yes, that was the original code. But the issue is that that
code was no longer correct when
[KIP-734](https://cwiki.apache.org/confluence/display/KAFKA/KIP-734%3A+Improve+AdminClient.listOffsets+to+return+timestamp+and+offset+for+the+record+with+the+largest+timestamp)
was added. Without KIP-734, it's ok to set offsetOfMaxTimestamp to the last
offset for magic V2. However, with KIP-734, it's important to set
offsetOfMaxTimestamp to the first offset for both magic V1 and V2. It seems
that we missed that part when adding KIP-734. So, I think the logic should be
if `timestampType == TimestampType.LOG_APPEND_TIME`, we always set
`offsetOfMaxTimestamp` to `initialOffset`.
--
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
