Thanks Jason. Some comments below. > > Generally the number of snapshots on disk will be one. I suspect that > users will want some control over this. We can add a configuration > option that doesn't delete, or advances the log begin offset past, the > N latest snapshots. We can set the default value for this > configuration to two. What do you think? > > I know Zookeeper has a config like this, but I'm not sure how frequently it > is used. I would probably suggest we pick a good number of snapshots (maybe > just 1-2) and leave it out of the configs. >
Sounds good to me. If followers/observers are keeping up with the Leader, I think the description in section "When to Increase the Log Begin Offset" will lead to one snapshot on disk in the steady state. > > We could use the same configuration we have for Fetch but to avoid > confusion let's add two more configurations for > "replica.fetch.snapshot.max.bytes" and > "replica.fetch.snapshot.response.max.bytes". > > My vote would probably be to reuse the existing configs. We can add new > configs in the future if the need emerges, but I can't think of a good > reason why a user would want these to be different. Sounds good to me. Removed the configuration from the KIP. Updated the FetchSnapshot request handling section to mention that the replica.fetch.response.max.bytes configuration will be used. > By the way, it looks like the FetchSnapshot schema now has both a partition > level and a top level max bytes. Do we need both? Kepted the top level MaxBytes and remove the topic partition level MaxBytes. > > The snapshot epoch will be used when ordering snapshots and more > importantly when setting the LastFetchedEpoch in the Fetch request. It > is possible for a follower to have a snapshot and an empty log. In > this case the follower will use the epoch of the snapshot when setting > the LastFetchEpoch in the Fetch request. > > Just to be clear, I think it is important to include the snapshot epoch so > that we /can/ reason about the snapshot state in the presence of data loss. > However, if we excluded data loss, then this would strictly speaking be > unnecessary because a snapshot offset would always be uniquely defined > (since we do not snapshot above the high watermark). Hence it would be safe > to leave LastFetchedEpoch undefined. Anyway, I think we're on the same page > about the behavior, just thought it might be useful to clarify the > reasoning. Okay. Even though you are correct that the LastFetchEpoch shouldn't matter since the follower is fetching committed data. I still think that the follower should send the epoch of the snapshot for the LastFetchedEpoch for extra validation on the leader. What do you think?
