junrao commented on code in PR #15621: URL: https://github.com/apache/kafka/pull/15621#discussion_r1548322220
########## clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java: ########## @@ -240,25 +240,40 @@ public MemoryRecords build() { return builtRecords; } + /** - * Get the max timestamp and its offset. The details of the offset returned are a bit subtle. - * Note: The semantic for the offset of max timestamp is the first offset with the max timestamp if there are multi-records having same timestamp. - * - * If the log append time is used, the offset will be the first offset of the record. - * - * If create time is used, the offset will always be the offset of the record with the max timestamp. - * - * If it's NO_TIMESTAMP (i.e. MAGIC_VALUE_V0), we'll return offset -1 since no timestamp info in records. + * We want to align the shallowOffsetOfMaxTimestamp for all paths (leader append, follower append, and index recovery) + * The definition of shallowOffsetOfMaxTimestamp is the last offset of the batch having max timestamp. + * If there are many batches having same max timestamp, we pick up the earliest batch. + * <p> + * If the log append time is used, the offset will be the last offset unless no compression is used and + * the message format version is 0 or 1, in which case, it will be -1. Review Comment: Still not very accurate. For message format version 0, the offset will be -1. For message format version 1, the offset will be the first offset. ########## clients/src/main/java/org/apache/kafka/common/record/MemoryRecordsBuilder.java: ########## @@ -240,25 +240,40 @@ public MemoryRecords build() { return builtRecords; } + /** - * Get the max timestamp and its offset. The details of the offset returned are a bit subtle. - * Note: The semantic for the offset of max timestamp is the first offset with the max timestamp if there are multi-records having same timestamp. - * - * If the log append time is used, the offset will be the first offset of the record. - * - * If create time is used, the offset will always be the offset of the record with the max timestamp. - * - * If it's NO_TIMESTAMP (i.e. MAGIC_VALUE_V0), we'll return offset -1 since no timestamp info in records. + * We want to align the shallowOffsetOfMaxTimestamp for all paths (leader append, follower append, and index recovery) + * The definition of shallowOffsetOfMaxTimestamp is the last offset of the batch having max timestamp. + * If there are many batches having same max timestamp, we pick up the earliest batch. + * <p> + * If the log append time is used, the offset will be the last offset unless no compression is used and + * the message format version is 0 or 1, in which case, it will be -1. + * <p> + * If create time is used, the offset will be the last offset unless no compression is used and the message + * format version is 0 or 1, in which case, it will be the offset of the record with the max timestamp. Review Comment: For message format version 0, the offset will be -1. For message format version 1, the offset will be the offset of the record with the max timestamp. -- 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