niket-goel commented on code in PR #12457:
URL: https://github.com/apache/kafka/pull/12457#discussion_r933489518
##########
raft/src/main/java/org/apache/kafka/snapshot/RecordsSnapshotReader.java:
##########
@@ -106,6 +106,7 @@ public static <T> RecordsSnapshotReader<T> of(
BufferSupplier bufferSupplier,
int maxBatchSize
) {
+ // TODO: Is this a good place to perform delimeter check (i.e.
existence of header and footer?)
Review Comment:
That is the downside, yes. We could also seek to the end of the snapshot
file and just read the first and last batch. More ideal w.r.t performance would
be to just check for the existence of the footer as you are iterating through
the snapshot. You would still realize that you have an incomplete snapshot, but
you would have applied a part of that incomplete snapshot to your memory (not
sure if that poses a correctness risk).
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1216,7 +1216,16 @@ class UnifiedLog(@volatile var logStartOffset: Long,
case FetchHighWatermark => fetchHighWatermarkMetadata
case FetchTxnCommitted => fetchLastStableOffsetMetadata
}
- localLog.read(startOffset, maxLength, minOneMessage, maxOffsetMetadata,
isolation == FetchTxnCommitted)
+ val fetchDataInfo = localLog.read(startOffset, maxLength, minOneMessage,
maxOffsetMetadata, isolation == FetchTxnCommitted)
Review Comment:
That is a fair question. I was contemplating that too. The alternative of
having a reader verify the bytes places less stress on the log layer in Kafka,
but is not really fool proof.
e.g. in that scheme the listeners that are reading the batches (both for
controller and broker) would do the CRC check (assuming the batch header
reaches that layer).
Actually now that I think about it, does having the check in the
`RecordsIterator` protect all readers?
--
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]
