chia7712 commented on code in PR #15476:
URL: https://github.com/apache/kafka/pull/15476#discussion_r1522318718
##########
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:
It seems KIP-734 does not define the case we are discussing - return the
initial/last offset when there are multi-records having same "max timestamp".
So how we deal with it? Is it a good practice to discuss compatibility? Or
we deal with it as "bug fix" and we add enough document to explain the "correct
behavior" ?
I prefer the consistent behavior. Hence, it seems to me that adding document
to `OffsetSpec.MaxTimestampSpec` is enough here. The use cases which depends on
previous behavior (last offset) should be re-written for this bug fix ( and
behavior change).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
