hachikuji commented on a change in pull request #9512: URL: https://github.com/apache/kafka/pull/9512#discussion_r518882604
########## File path: raft/src/main/java/org/apache/kafka/snapshot/SnapshotReader.java ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.snapshot; + +import java.io.Closeable; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.Iterator; +import org.apache.kafka.common.record.RecordBatch; +import org.apache.kafka.raft.OffsetAndEpoch; + +// TODO: Write documentation for this type and all of the methods +public interface SnapshotReader extends Closeable, Iterable<RecordBatch> { Review comment: Ok, I think I see what you're saying. I guess I was not expecting that we would need a separate low-level interface when we already have `FileRecords` and `MemoryRecords`. I think `Records` already gives us a way to read from arbitrary positions: ```java long writeTo(GatheringByteChannel channel, long position, int length) throws IOException; ``` But there is no `Records` implementation currently that allows unaligned writes. We only have this: ```java public int append(MemoryRecords records) throws IOException; ``` Perhaps it would be possible to extend `Records` to provide what we need instead of creating a new interface? As far as the naming, I wonder if we can reserve the nicer `SnapshotXXX` names for the state machine. Really what I would like is a common type that can be used by both `handleSnapshot` and `handleCommit` since both callbacks just need to provide a way to read through a set of records. I was thinking it could be `BatchReader`, but it doesn't have to be if there is something better. (By the way, it's not too clear to me why we need `freeze` when we already have `close`.) ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
