+1 This is a very well-written KIP! Minor: there is still a mix of terms in the doc that references the earlier LeaderGenerationRequest (which is what I'm assuming what it was called in previous versions of the wiki). Same for the diagrams which I'm guessing are a little harder to make consistent with the text.
On Thu, Jan 5, 2017 at 5:54 PM, Jun Rao <j...@confluent.io> wrote: > Hi, Ben, > > Thanks for the updated KIP. +1 > > 1) In OffsetForLeaderEpochResponse, start_offset probably should be > end_offset since it's the end offset of that epoch. > 3) That's fine. We can fix KAFKA-1120 separately. > > Jun > > > On Thu, Jan 5, 2017 at 11:11 AM, Ben Stopford <b...@confluent.io> wrote: > > > Hi Jun > > > > Thanks for raising these points. Thorough as ever! > > > > 1) Changes made as requested. > > 2) Done. > > 3) My plan for handing returning leaders is to simply to force the Leader > > Epoch to increment if a leader returns. I don't plan to fix KAFKA-1120 as > > part of this KIP. It is really a separate issue with wider implications. > > I'd be happy to add KAFKA-1120 into the release though if we have time. > > 4) Agreed. Not sure exactly how that's going to play out, but I think > we're > > on the same page. > > > > Please could > > > > Cheers > > B > > > > On Thu, Jan 5, 2017 at 12:50 AM Jun Rao <j...@confluent.io> wrote: > > > > > Hi, Ben, > > > > > > Thanks for the proposal. Looks good overall. A few comments below. > > > > > > 1. For LeaderEpochRequest, we need to include topic right? We probably > > want > > > to follow other requests by nesting partition inside topic? For > > > LeaderEpochResponse, > > > do we need to return leader_epoch? I was thinking that we could just > > return > > > an end_offset, which is the next offset of the last message in the > > > requested leader generation. Finally, would OffsetForLeaderEpochRequest > > be > > > a better name? > > > > > > 2. We should bump up both the produce request and the fetch request > > > protocol version since both include the message set. > > > > > > 3. Extending LeaderEpoch to include Returning Leaders: To support this, > > do > > > you plan to use the approach that stores CZXID in the broker > > registration > > > and including the CZXID of the leader in /brokers/topics/[topic]/ > > > partitions/[partitionId]/state in ZK? > > > > > > 4. Since there are a few other KIPs involving message format too, it > > would > > > be useful to consider if we could combine the message format changes in > > the > > > same release. > > > > > > Thanks, > > > > > > Jun > > > > > > > > > On Wed, Jan 4, 2017 at 9:24 AM, Ben Stopford <b...@confluent.io> wrote: > > > > > > > Hi All > > > > > > > > We’re having some problems with this thread being subsumed by the > > > > [Discuss] thread. Hopefully this one will appear distinct. If you see > > > more > > > > than one, please use this one. > > > > > > > > KIP-101 should now be ready for a vote. As a reminder the KIP > proposes > > a > > > > change to the replication protocol to remove the potential for > replicas > > > to > > > > diverge. > > > > > > > > The KIP can be found here: https://cwiki.apache.org/confl > > > > uence/display/KAFKA/KIP-101+-+Alter+Replication+Protocol+to+ > > > > use+Leader+Epoch+rather+than+High+Watermark+for+Truncation < > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-101+- > > > > +Alter+Replication+Protocol+to+use+Leader+Epoch+rather+ > > > > than+High+Watermark+for+Truncation> > > > > > > > > Please let us know your vote. > > > > > > > > B > > > > > > > > > > > > > > > > > > > > > > > > > >
