KAFKA-3755 Tightening the offset check in ReplicaFetcherThread #1506
Conversation
Including additional check to make sure that the first offset in the message set to be appended to the log is >= than the log end offset.
|
Thanks for the PR, it would be good to include a test that validates the change. |
| @@ -119,6 +119,8 @@ class ReplicaFetcherThread(name: String, | |||
|
|
|||
| if (fetchOffset != replica.logEndOffset.messageOffset) | |||
| throw new RuntimeException("Offset mismatch for partition %s: fetched offset = %d, log end offset = %d.".format(topicAndPartition, fetchOffset, replica.logEndOffset.messageOffset)) | |||
| if (!messageSet.isEmpty && messageSet.head.offset < replica.logEndOffset.messageOffset) | |||
| throw new RuntimeException("Offset error for partition %s: first offset in message set = %d, must not be less than log end offset = %d.".format(topicAndPartition, messageSet.head.offset, replica.logEndOffset.messageOffset)) | |||
There was a problem hiding this comment.
Nitpick: it would be good to break the line so that the line length fits within the GitHub review window.
There was a problem hiding this comment.
Sure, I'll have the .format() part in the next line in the next patch set with the test case. Thanks
There was a problem hiding this comment.
@imandhan : Instead of adding the checking here, it's probably better to do that in Log.append(). In the path where assignOffsets is false, we can just require that appendInfo.firstOffset >= nextOffsetMetadata.messageOffset. This also makes testing easier since it can be done on an instance of Log.
There was a problem hiding this comment.
@junrao looks like Log.append() already has that check. See https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/log/Log.scala#L370 Am I misunderstanding something here?
There was a problem hiding this comment.
@imandhan : Yes, you are right. I didn't realize that we already had a check there and a unit test for it. Sorry for the confusion. I will close the jira and mark it as a non-issue. Hopefully that you can find some other Kafka jiras to contribute.
|
@ijuma Sure, I can add a test case but am not sure what file to add it to since there doesn't seem to be a test file corresponding to ReplicaFetcherThread. |
|
Since we are not doing this anymore, can you please close the PR? (we can't close it ourselves). |
|
Done |
Including an additional check to make sure that the first offset in the message set to be appended to the log is >= than the log end offset.