cmccabe commented on code in PR #13345: URL: https://github.com/apache/kafka/pull/13345#discussion_r1146542326
########## raft/src/main/java/org/apache/kafka/snapshot/RecordsSnapshotReader.java: ########## @@ -121,9 +122,22 @@ private Optional<Batch<T>> nextBatch() { Batch<T> batch = iterator.next(); if (!lastContainedLogTimestamp.isPresent()) { - // The Batch type doesn't support returning control batches. For now lets just use - // the append time of the first batch - lastContainedLogTimestamp = OptionalLong.of(batch.appendTimestamp()); + // This must be the first batch which is expected to be a control batch with one record for + // the snapshot header. + if (batch.controlRecords().isEmpty()) { + throw new IllegalStateException("First batch is not a control batch with at least one record"); Review Comment: This will break compatibility with older snapshot versions. Certainly it breaks compatibility with the oldest raft snapshots, which had no control records. I don't recall the exact version where we added the initial control record. You need to find that version and then we can discuss whether breaking compatibility here is acceptable. If it is, we need to have an appropriate error message that talks about why the older version is not supported and which versions are supported. -- 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