jsancio commented on a change in pull request #9512: URL: https://github.com/apache/kafka/pull/9512#discussion_r518916392
########## 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: > Perhaps it would be possible to extend Records to provide what we need instead of creating a new interface? I think you are right that we don't need `SnapshotReader` as `Records` provides all of the functionality we need. `SnapshotReader` was added so that we didn't expose all of the mutating APIs in `FileRecords`. Snapshot are supposed to be immutable once frozen. I think we still want `SnapshotWriter` or something similar as it provides 1. raw writes of bytes and 2. `freeze` which optionally marks the snapshot as immutable if the validation passes. For 1., I don't think we should add raw writes to `Records` or `FileRecords` as in essence we are exposing an unsafe API and the user needs to make sure that they are writing the correct data. For 2., I think we can get away from introducing a new type/interface and instead add that functionality as a static method in `Snapshots.java`. Unittest (mock tests) maybe difficult with this code organization. > As far as the naming, I wonder if we can reserve the nicer SnapshotXXX names for the state machine. Yes.I'll use the prettier name for the type exposed to 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. I think this should be possible. I haven't implemented this part so I don't have all of the details. I think the requirement for the type sent through `handleSnapshot` are a subset of the requirements for the type sent through `handleCommit`. In particular, the Raft Client (`ListenerContext`) only needs to know when the snapshot has been read fully (e.g. `close`). The Raft Client doesn't need so to know what is the "last seen offset + 1" as it already knows the `SnapshotId` sent through `handleSnapshot`. > (By the way, it's not too clear to me why we need freeze when we already have close.) I wanted to allow the user to abort a snapshot. If `close` is called without calling `freeze` then the partial snapshot is deleted and not made immutable. ---------------------------------------------------------------- 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
