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


Reply via email to