junrao commented on code in PR #15621:
URL: https://github.com/apache/kafka/pull/15621#discussion_r1548211287
##########
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 which having max timestamp.
Review Comment:
which having => having
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogValidator.java:
##########
@@ -293,14 +293,29 @@ public ValidationResult
assignOffsetsNonCompressed(LongRef offsetCounter,
if (timestampType == TimestampType.LOG_APPEND_TIME) {
maxTimestamp = now;
- offsetOfMaxTimestamp = initialOffset;
+ // those checks should be equal to MemoryRecordsBuilder#info
+ switch (toMagic) {
+ case RecordBatch.MAGIC_VALUE_V0:
+ // value will be the default value: -1
+ shallowOffsetOfMaxTimestamp = -1;
Review Comment:
maxTimestamp should be NO_TIMESTAMP if magic is 0.
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogValidator.java:
##########
@@ -434,7 +442,8 @@ public ValidationResult
validateMessagesAndAssignOffsetsCompressed(LongRef offse
now,
records,
maxTimestamp,
- offsetOfMaxTimestamp,
+ // there is only one batch in this path, so last offset can be
viewed as shallowOffsetOfMaxTimestamp
+ lastOffset,
Review Comment:
If magic is 0, we should set both maxTimestamp and
shallowOffsetOfMaxTimestamp to -1.
##########
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 which 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 the
first offset.
Review Comment:
For message format 0, offset is always -1. Ditto below.
--
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
