Hi Jun, Thank you for taking the time to review the KIP. Please find my comments inline.
On Fri, Apr 5, 2024 at 12:09 AM Jun Rao <j...@confluent.io.invalid> wrote: > Hi, Abhijeet, > > Thanks for the KIP. Left a few comments. > > 1. "A drawback of using the last-tiered-offset is that this new follower > would possess only a limited number of locally stored segments. Should it > ascend to the role of leader, there is a risk of needing to fetch these > segments from the remote storage, potentially impacting broker > performance." > Since we support consumers fetching from followers, this is a potential > issue on the follower side too. In theory, it's possible for a segment to > be tiered immediately after rolling. In that case, there could be very > little data after last-tiered-offset. It would be useful to think through > how to address this issue. > We plan to have a follow-up KIP that will address both the deprioritization of these brokers from leadership, as well as for consumption (when fetching from followers is allowed). > > 2. ListOffsetsRequest: > 2.1 Typically, we need to bump up the version of the request if we add a > new value for timestamp. See > > https://github.com/apache/kafka/pull/10760/files#diff-fac7080d67da905a80126d58fc1745c9a1409de7ef7d093c2ac66a888b134633 > . > Yes, let me update the KIP to include this change. We will need a new timestamp corresponding to Earliest-Pending-Upload-Offset. > 2.2 Since this changes the inter broker request protocol, it would be > useful to have a section on upgrade (e.g. new IBP/metadata.version). > > Make sense. I will update the KIP to capture this. > 3. "Instead of fetching Earliest-Pending-Upload-Offset, it could fetch the > last-tiered-offset from the leader, and make a separate leader call to > fetch leader epoch for the following offset." > Why do we need to make a separate call for the leader epoch? > ListOffsetsResponse include both the offset and the corresponding epoch. > > I understand there is some confusion here. Let me try to explain this. The follower needs to build the local data starting from the offset Earliest-Pending-Upload-Offset. Hence it needs the offset and the corresponding leader-epoch. There are two ways to do this: 1. We add support in ListOffsetRequest to be able to fetch this offset (and leader epoch) from the leader. 2. Or, fetch the tiered-offset (which is already supported). From this offset, we can get the Earliest-Pending-Upload-Offset. We can just add 1 to the tiered-offset. However, we still need the leader epoch for offset, since there is no guarantee that the leader epoch for Earliest-Pending-Upload-Offset will be the same as the leader epoch for tiered-offset. We may need another API call to the leader for this. I prefer the first approach. The only problem with the first approach is that it introduces one more offset. The second approach avoids this problem but is a little complicated. > 4. "Check if the follower replica is empty and if the feature to use > last-tiered-offset is enabled." > Why do we need to check if the follower replica is empty? > > We want to limit this new behavior only to new replicas. Replicas that become out of ISR are excluded from this behavior change. Those will continue with the existing behavior. > 5. It can be confirmed by checking if the leader's Log-Start-Offset is the > same as the Leader's Local-Log-Start-Offset. > How does the follower know Local-Log-Start-Offset? > Missed this detail. The follower will need to call the leader APi to fetch the EarliestLocal offset for this. > Jun > > On Sat, Mar 30, 2024 at 5:51 AM Abhijeet Kumar <abhijeet.cse....@gmail.com > > > wrote: > > > Hi Christo, > > > > Thanks for reviewing the KIP. > > > > The follower needs the earliest-pending-upload-offset (and the > > corresponding leader epoch) from the leader. > > This is the first offset the follower will have locally. > > > > Regards, > > Abhijeet. > > > > > > > > On Fri, Mar 29, 2024 at 1:14 PM Christo Lolov <christolo...@gmail.com> > > wrote: > > > > > Heya! > > > > > > First of all, thank you very much for the proposal, you have explained > > the > > > problem you want solved very well - I think a faster bootstrap of an > > empty > > > replica is definitely an improvement! > > > > > > For my understanding, which concrete offset do you want the leader to > > give > > > back to a follower - earliest-pending-upload-offset or the > > > latest-tiered-offset? If it is the second, then I believe KIP-1005 > ought > > to > > > already be exposing that offset as part of the ListOffsets API, no? > > > > > > Best, > > > Christo > > > > > > On Wed, 27 Mar 2024 at 18:23, Abhijeet Kumar < > abhijeet.cse....@gmail.com > > > > > > wrote: > > > > > > > Hi All, > > > > > > > > I have created KIP-1023 to introduce follower fetch from tiered > offset. > > > > This feature will be helpful in significantly reducing Kafka > > > > rebalance/rebuild times when the cluster is enabled with tiered > > storage. > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1023%3A+Follower+fetch+from+tiered+offset > > > > > > > > Feedback and suggestions are welcome. > > > > > > > > Regards, > > > > Abhijeet. > > > > > > > > > >
