Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
MIME-Version: 1.0
In-Reply-To: <CACBUTrdwDs01rHeTL6TwxxhnfV=VXsqQPhUSdBy81KURSa+dRg@mail.gmail.com>
References: <CAAaarBZ7KT4GKFF6t_BGenBbOPTwtjYHSk79+bQvvBAD0mmw=g@mail.gmail.com>
 <CAFc58G_-+fzOy6xvVFaJL+F1AOSCWKp4Z6pRNbgtsFmxf8Vn_g@mail.gmail.com>
 <CAAaarBYTp0s_DffKNCpbekfYYtG6Livxsstb8STngTCi9SQm3A@mail.gmail.com>
 <CAFc58G_1FHzj-M=KScBBakn6=FSSU4L81EdF7ndhrPo62gfs_A@mail.gmail.com>
 <CAAaarBYdgbm4Vj+sBJTS8i5p5Hpq9_35yeYjPHjgXQXTT_P=EQ@mail.gmail.com>
 <CAFc58G-TjL9q1FmYQYvxpTEuqzsMo8tAnKX8uQRUBb=OjsomXg@mail.gmail.com>
 <CAAaarBbhgC4egQyxx4EOQE7B1RWFe_+0WPJZ_A7Jm6iWE9T=Lw@mail.gmail.com>
 <CAHwHRrW2S1wV8gOS0-U1q8NEg3a-Rirfb2XvikV5L3fkzLEnqw@mail.gmail.com>
 <CAAaarBZaxjODqPqPikWyfyC_3iOe-vunuzot0Q+7yJBE3Jud0A@mail.gmail.com>
 <CAHwHRrXGqndzxzuxfScnD2s1VVA8c3k8uv_1v_axXZOi0r5piA@mail.gmail.com>
 <CAAaarBZXPFGLFg0UzR3udZcPrhHqfZRE9FL8Gp-Qd8JQEVTE0A@mail.gmail.com>
 <CAHwHRrXdGqZA=B_8zSkhyp6mUFAU28W-HGLSQmBceHdAG-0iPw@mail.gmail.com>
 <CAAaarBahiJk7n8f_L=nd+zW=sM__dmV0DaG5ZM8r3VO+h4R=eQ@mail.gmail.com>
 <CAFc58G9VJD=WzJrkniFsmZEPmzeU-Ver9r83nMBNPSB7DcQTSA@mail.gmail.com>
 <CAAaarBZgay9KdX4L_j9aL8FGQLzcngcZUhUDP=442PJ-061-SQ@mail.gmail.com>
 <CAAaarBa45uuaimmL8K3vTY3rMEy2rTd6_QaAkG6p9=q-xSobcw@mail.gmail.com>
 <CAFc58G_rBa68sfsFVq3WwhY0HCQ8dyRQUVeR5EKhpRLqUwuw6Q@mail.gmail.com> <CACBUTrdwDs01rHeTL6TwxxhnfV=VXsqQPhUSdBy81KURSa+dRg@mail.gmail.com>
From: Dong Lin <lindong28@gmail.com>
Date: Tue, 31 Jan 2017 17:47:39 -0800
Message-ID: <CAAaarBY9axhc+gG0sZNTV7URQ1mC2cPDcR59S5vBn_xpq3-KQQ@mail.gmail.com>
Subject: Re: [VOTE] KIP-107 Add purgeDataBefore() API in AdminClient
To: dev@kafka.apache.org
Content-Type: multipart/alternative; boundary=f403045e6ff80cd84805476e3ddf
archived-at: Wed, 01 Feb 2017 01:48:20 -0000

--f403045e6ff80cd84805476e3ddf
Content-Type: text/plain; charset=UTF-8

Hey Apurva,

I think the KIP table in
https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals
has already been updated. Is there anything I missed?

Thanks,
Dong

On Tue, Jan 31, 2017 at 5:45 PM, Apurva Mehta <apurva@confluent.io> wrote:

> Hi Dong,
>
> It looks like this vote passed. Can you close this thread and update the
> KIP table?
>
> Thanks,
> Apurva
>
> On Tue, Jan 24, 2017 at 1:30 PM, Jun Rao <jun@confluent.io> wrote:
>
> > Hi, Dong,
> >
> > The changes sound good to me. Also, thanks for the explanation of
> returning
> > a future from purgeDataFrom(). We can keep it that way.
> >
> > Thanks,
> >
> > Jun
> >
> > On Mon, Jan 23, 2017 at 4:24 PM, Dong Lin <lindong28@gmail.com> wrote:
> >
> > > Hi all,
> > >
> > > When I am implementing the patch, I realized that the current usage of
> > > "low_watermark" is a bit confusing. So I made the following interface
> > > changes in the KIP:
> > >
> > > - The newly added checkpoint file will be named
> > log-begin-offset-checkpoint
> > > - Replace low_watermark with log_begin_offset in FetchRequestPartition
> > and
> > > FetchResponsePartitionHeader
> > >
> > > The problem with the previous naming conversion is that, low_watermark
> > > implies minimum log begin offset of all replicas (similar to high
> > > watermark) and we return this value in the PurgeResponse. In other
> words,
> > > low_watermark can not be incremented if a follower is not live.
> Therefore
> > > we can not use low_watermark in the checkpoint file or in the
> > FetchResponse
> > > from leader to followers if we want to persists the offset-to-purge
> > > received from user across broker rebounce.
> > >
> > > You can find the changes in KIP here
> > > <https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?
> > > pageId=67636826&selectedPageVersions=13&selectedPageVersions=14>.
> > > Please let me know if you have any concern with this change.
> > >
> > > Thanks,
> > > Dong
> > >
> > > On Mon, Jan 23, 2017 at 11:20 AM, Dong Lin <lindong28@gmail.com>
> wrote:
> > >
> > > > Thanks for the comment Jun.
> > > >
> > > > Yeah, I think there is use-case where this can be useful. Allowing
> for
> > > > asynchronous delete will be useful if an application doesn't need
> > strong
> > > > guarantee of purgeDataFrom(), e.g. if it is done to help reduce disk
> > > usage
> > > > of kafka. The application may want to purge data for every time it
> does
> > > > auto-commit without wait for future object to complete. On the other
> > > hand,
> > > > synchronous delete will be useful if an application wants to make
> sure
> > > that
> > > > the sensitive or bad data is definitely deleted. I think returning a
> > > future
> > > > makes both choice available to user and it doesn't complicate
> > > > implementation much.
> > > >
> > > >
> > > > On Mon, Jan 23, 2017 at 10:45 AM, Jun Rao <jun@confluent.io> wrote:
> > > >
> > > >> I feel that it's simpler to just keep the format of the checkpoint
> > file
> > > as
> > > >> it is and just add a separate checkpoint for low watermark. Low
> > > watermark
> > > >> and high watermark are maintained independently. So, not sure if
> there
> > > is
> > > >> significant benefit of storing them together.
> > > >>
> > > >> Looking at the KIP again. I actually have another question on the
> api.
> > > Is
> > > >> there any benefit of returning a Future in the purgeDataBefore()
> api?
> > > >> Since
> > > >> admin apis are used infrequently, it seems that it's simpler to just
> > > have
> > > >> a
> > > >> blocking api and returns Map<TopicPartition, PurgeDataResult>?
> > > >>
> > > >> Thanks,
> > > >>
> > > >> Jun
> > > >>
> > > >> On Sun, Jan 22, 2017 at 3:56 PM, Dong Lin <lindong28@gmail.com>
> > wrote:
> > > >>
> > > >> > Thanks for the comment Guozhang. Please don't worry about being
> > late.
> > > I
> > > >> > would like to update the KIP if there is clear benefit of the new
> > > >> approach.
> > > >> > I am wondering if there is any use-case or operation aspects that
> > > would
> > > >> > benefit from the new approach.
> > > >> >
> > > >> > I am not saying that these checkpoint files have the same
> priority.
> > I
> > > >> > mentioned other checkpoint files to suggest that it is OK to add
> one
> > > >> more
> > > >> > checkpoint file. To me three checkpoint files is not much
> different
> > > from
> > > >> > four checkpoint files. I am just inclined to not update the KIP if
> > the
> > > >> only
> > > >> > benefit is to avoid addition of a new checkpoint file.
> > > >> >
> > > >> >
> > > >> >
> > > >> > On Sun, Jan 22, 2017 at 3:48 PM, Guozhang Wang <
> wangguoz@gmail.com>
> > > >> wrote:
> > > >> >
> > > >> > > To me the distinction between recovery-checkpoint and
> > > >> > > replication-checkpoint are different from the distinction
> between
> > > >> these
> > > >> > two
> > > >> > > hw checkpoint values: when broker starts up and act as the
> leader
> > > for
> > > >> a
> > > >> > > partition, it can live without seeing the recovery checkpoint,
> but
> > > >> just
> > > >> > > cannot rely on the existing last log segment and need to fetch
> > from
> > > >> other
> > > >> > > replicas; but if the replication-checkpoint file is missing, it
> > is a
> > > >> > > correctness issue, as it does not know from where to truncate
> its
> > > >> data,
> > > >> > and
> > > >> > > also how to respond to a fetch request. That is why I think we
> can
> > > >> > separate
> > > >> > > these two types of files, since the latter one is more important
> > > than
> > > >> the
> > > >> > > previous one.
> > > >> > >
> > > >> > > That being said, I do not want to recall another vote on this
> > since
> > > >> it is
> > > >> > > my bad not responding before the vote is called. Just wanted to
> > > point
> > > >> out
> > > >> > > for the record that this approach may have some operational
> > > scenarios
> > > >> > where
> > > >> > > one of the replication files is missing and we need to treat
> them
> > > >> > > specifically.
> > > >> > >
> > > >> > >
> > > >> > > Guozhang
> > > >> > >
> > > >> > >
> > > >> > > On Sun, Jan 22, 2017 at 1:56 PM, Dong Lin <lindong28@gmail.com>
> > > >> wrote:
> > > >> > >
> > > >> > > > Yeah, your solution of adding new APIs certainly works and I
> > don't
> > > >> > think
> > > >> > > > that is an issue. On the other hand I don't think it is an
> issue
> > > to
> > > >> > add a
> > > >> > > > new checkpoint file as well since we already have multiple
> > > >> checkpoint
> > > >> > > > files. The benefit of the new approach you mentioned is
> probably
> > > >> not an
> > > >> > > > issue in the current approach since high watermark and low
> > > watermark
> > > >> > > works
> > > >> > > > completely independently. Since there is no strong reason to
> > > choose
> > > >> > > either
> > > >> > > > of them, I am inclined to choose the one that makes less
> format
> > > >> change
> > > >> > > and
> > > >> > > > simpler in the Java API. The current approach seems better
> w.r.t
> > > >> this
> > > >> > > minor
> > > >> > > > reason.
> > > >> > > >
> > > >> > > > If you are strong that we should use the new approach, I can
> do
> > > >> that as
> > > >> > > > well. Please let me know if you think so, and I will need to
> ask
> > > >> > > > Jun/Joel/Becket to vote on this again since this changes the
> > > >> interface
> > > >> > of
> > > >> > > > the KIP.
> > > >> > > >
> > > >> > > > On Sun, Jan 22, 2017 at 9:35 AM, Guozhang Wang <
> > > wangguoz@gmail.com>
> > > >> > > wrote:
> > > >> > > >
> > > >> > > > > I think this is less of an issue: we can use the same
> patterns
> > > as
> > > >> in
> > > >> > > the
> > > >> > > > > request protocol, i.e.:
> > > >> > > > >
> > > >> > > > > write(Map[TP, Long]) // write the checkout point in v0
> format
> > > >> > > > > write(Map[TP, Pair[Long, Long]]) // write the checkout point
> > in
> > > v1
> > > >> > > format
> > > >> > > > >
> > > >> > > > > CheckpointedOffsets read() // read the file relying on its
> > > >> version id
> > > >> > > > >
> > > >> > > > > class CheckpointedOffsets {
> > > >> > > > >
> > > >> > > > >     Integer getVersion();
> > > >> > > > >     Long getFirstOffset();
> > > >> > > > >     Long getSecondOffset();   // would return NO_AVAILABLE
> > with
> > > v0
> > > >> > > format
> > > >> > > > > }
> > > >> > > > >
> > > >> > > > >
> > > >> > > > > As I think of it, another benefit is that we wont have a
> > > partition
> > > >> > that
> > > >> > > > > only have one of the watermarks in case of a failure in
> > between
> > > >> > writing
> > > >> > > > two
> > > >> > > > > files.
> > > >> > > > >
> > > >> > > > > Guozhang
> > > >> > > > >
> > > >> > > > > On Sun, Jan 22, 2017 at 12:03 AM, Dong Lin <
> > lindong28@gmail.com
> > > >
> > > >> > > wrote:
> > > >> > > > >
> > > >> > > > > > Hey Guozhang,
> > > >> > > > > >
> > > >> > > > > > Thanks for the review:) Yes it is possible to combine
> them.
> > > Both
> > > >> > > > solution
> > > >> > > > > > will have the same performance. But I think the current
> > > solution
> > > >> > will
> > > >> > > > > give
> > > >> > > > > > us simpler Java class design. Note that we will have to
> > change
> > > >> Java
> > > >> > > API
> > > >> > > > > > (e.g. read() and write()) of OffsetCheckpoint class in
> order
> > > to
> > > >> > > > provide a
> > > >> > > > > > map from TopicPartition to a pair of integers when we
> write
> > to
> > > >> > > > checkpoint
> > > >> > > > > > file. This makes this class less generic since this API is
> > not
> > > >> used
> > > >> > > by
> > > >> > > > > log
> > > >> > > > > > recovery checkpoint and log cleaner checkpoint which are
> > also
> > > >> using
> > > >> > > > > > OffsetCheckpoint class.
> > > >> > > > > >
> > > >> > > > > > Dong
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > >
> > > >> > > > > > On Sat, Jan 21, 2017 at 12:28 PM, Guozhang Wang <
> > > >> > wangguoz@gmail.com>
> > > >> > > > > > wrote:
> > > >> > > > > >
> > > >> > > > > > > Hi Dong,
> > > >> > > > > > >
> > > >> > > > > > > Sorry for being late on reviewing this KIP. It LGTM
> > overall,
> > > >> but
> > > >> > > I'm
> > > >> > > > > > > wondering if we can save adding the
> > > >> "replication-low-watermark-
> > > >> > > > > > checkpoint"
> > > >> > > > > > > file by just bumping up the version number of
> > > >> > "replication-offset-
> > > >> > > > > > > checkpoint"
> > > >> > > > > > > to let it have two values for each partition, i.e.:
> > > >> > > > > > >
> > > >> > > > > > > 1  // version number
> > > >> > > > > > > [number of partitions]
> > > >> > > > > > > [topic name] [partition id] [lwm] [hwm]
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > > This will affects the upgrade path a bit, but I think
> not
> > by
> > > >> > large,
> > > >> > > > and
> > > >> > > > > > all
> > > >> > > > > > > other logic will not be affected.
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > > Guozhang
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > > On Wed, Jan 18, 2017 at 6:12 PM, Dong Lin <
> > > >> lindong28@gmail.com>
> > > >> > > > wrote:
> > > >> > > > > > >
> > > >> > > > > > > > Thanks to everyone who voted and provided feedback!
> > > >> > > > > > > >
> > > >> > > > > > > > This KIP is now adopted with 3 binding +1s (Jun, Joel,
> > > >> Becket)
> > > >> > > and
> > > >> > > > 2
> > > >> > > > > > > > non-binding +1s (Radai, Mayuresh).
> > > >> > > > > > > >
> > > >> > > > > > > > Thanks,
> > > >> > > > > > > > Dong
> > > >> > > > > > > >
> > > >> > > > > > > > On Wed, Jan 18, 2017 at 6:05 PM, Jun Rao <
> > > jun@confluent.io>
> > > >> > > wrote:
> > > >> > > > > > > >
> > > >> > > > > > > > > Hi, Dong,
> > > >> > > > > > > > >
> > > >> > > > > > > > > Thanks for the update. +1
> > > >> > > > > > > > >
> > > >> > > > > > > > > Jun
> > > >> > > > > > > > >
> > > >> > > > > > > > > On Wed, Jan 18, 2017 at 1:44 PM, Dong Lin <
> > > >> > lindong28@gmail.com
> > > >> > > >
> > > >> > > > > > wrote:
> > > >> > > > > > > > >
> > > >> > > > > > > > > > Hi Jun,
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > After some more thinking, I agree with you that it
> > is
> > > >> > better
> > > >> > > to
> > > >> > > > > > > simply
> > > >> > > > > > > > > > throw OffsetOutOfRangeException and not update
> > > >> > low_watermark
> > > >> > > if
> > > >> > > > > > > > > > offsetToPurge is larger than high_watermark.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > My use-case of allowing low_watermark >
> > high_watermark
> > > >> in
> > > >> > > 2(b)
> > > >> > > > is
> > > >> > > > > > to
> > > >> > > > > > > > > allow
> > > >> > > > > > > > > > user to purge all the data in the log even if that
> > > data
> > > >> is
> > > >> > > not
> > > >> > > > > > fully
> > > >> > > > > > > > > > replicated to followers. An offset higher than
> > > >> > high_watermark
> > > >> > > > may
> > > >> > > > > > be
> > > >> > > > > > > > > > returned to user either through producer's
> > > >> RecordMetadata,
> > > >> > or
> > > >> > > > > > through
> > > >> > > > > > > > > > ListOffsetResponse if from_consumer option is
> false.
> > > >> > However,
> > > >> > > > > this
> > > >> > > > > > > may
> > > >> > > > > > > > > > cause problem in case of unclean leader election
> or
> > > when
> > > >> > > > consumer
> > > >> > > > > > > seeks
> > > >> > > > > > > > > to
> > > >> > > > > > > > > > the largest offset of the partition. It will
> > > complicate
> > > >> > this
> > > >> > > > KIP
> > > >> > > > > if
> > > >> > > > > > > we
> > > >> > > > > > > > > were
> > > >> > > > > > > > > > to address these two problems.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > At this moment I prefer to keep this KIP simple by
> > > >> > requiring
> > > >> > > > > > > > > low_watermark
> > > >> > > > > > > > > > <= high_watermark. The caveat is that if user does
> > > want
> > > >> to
> > > >> > > > purge
> > > >> > > > > > > *all*
> > > >> > > > > > > > > the
> > > >> > > > > > > > > > data that is already produced, then he needs to
> stop
> > > all
> > > >> > > > > producers
> > > >> > > > > > > that
> > > >> > > > > > > > > are
> > > >> > > > > > > > > > producing into this topic, wait long enough for
> all
> > > >> > followers
> > > >> > > > to
> > > >> > > > > > > catch
> > > >> > > > > > > > > up,
> > > >> > > > > > > > > > and then purge data using the latest offset of
> this
> > > >> > > partition,
> > > >> > > > > i.e.
> > > >> > > > > > > > > > high_watermark. We can revisit this if some strong
> > > >> use-case
> > > >> > > > comes
> > > >> > > > > > up
> > > >> > > > > > > in
> > > >> > > > > > > > > the
> > > >> > > > > > > > > > future.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > I also updated the KIP to allow user to use offset
> > -1L
> > > >> to
> > > >> > > > > indicate
> > > >> > > > > > > > > > high_watermark in the PurgeRequest. In the future
> we
> > > can
> > > >> > > allow
> > > >> > > > > > users
> > > >> > > > > > > to
> > > >> > > > > > > > > use
> > > >> > > > > > > > > > offset -2L to indicate that they want to purge all
> > > data
> > > >> up
> > > >> > to
> > > >> > > > > > > > > logEndOffset.
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > Thanks!
> > > >> > > > > > > > > > Dong
> > > >> > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > On Wed, Jan 18, 2017 at 10:37 AM, Jun Rao <
> > > >> > jun@confluent.io>
> > > >> > > > > > wrote:
> > > >> > > > > > > > > >
> > > >> > > > > > > > > > > Hi, Dong,
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > For 2(b), it seems a bit weird to allow
> > > highWatermark
> > > >> to
> > > >> > be
> > > >> > > > > > smaller
> > > >> > > > > > > > > than
> > > >> > > > > > > > > > > lowWatermark. Also, from the consumer's
> > perspective,
> > > >> > > messages
> > > >> > > > > are
> > > >> > > > > > > > > > available
> > > >> > > > > > > > > > > only up to highWatermark. What if we simply
> throw
> > > >> > > > > > > > > > OffsetOutOfRangeException
> > > >> > > > > > > > > > > if offsetToPurge is larger than highWatermark?
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Thanks,
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > Jun
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > On Tue, Jan 17, 2017 at 9:54 PM, Dong Lin <
> > > >> > > > lindong28@gmail.com
> > > >> > > > > >
> > > >> > > > > > > > wrote:
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > > > > Hi Jun,
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > Thank you. Please see my answers below. The
> KIP
> > is
> > > >> > > updated
> > > >> > > > to
> > > >> > > > > > > > answer
> > > >> > > > > > > > > > > these
> > > >> > > > > > > > > > > > questions (see here
> > > >> > > > > > > > > > > > <https://cwiki.apache.org/confluence/pages/
> > > >> > > > > > > > diffpagesbyversion.action
> > > >> > > > > > > > > ?
> > > >> > > > > > > > > > > > pageId=67636826&selectedPageVersions=5&
> > > >> > > > > selectedPageVersions=6>
> > > >> > > > > > > > > > > > ).
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > 1. Yes, in this KIP we wait for all replicas.
> > This
> > > >> is
> > > >> > the
> > > >> > > > > same
> > > >> > > > > > as
> > > >> > > > > > > > if
> > > >> > > > > > > > > > > > producer sends a messge with ack=all and
> > > >> > > isr=all_replicas.
> > > >> > > > So
> > > >> > > > > > it
> > > >> > > > > > > > > seems
> > > >> > > > > > > > > > > that
> > > >> > > > > > > > > > > > the comparison is OK?
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > 2. Good point! I haven't thought about the
> case
> > > >> where
> > > >> > the
> > > >> > > > > > > > > > user-specified
> > > >> > > > > > > > > > > > offset > logEndOffset. Please see answers
> below.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > a) If offsetToPurge < lowWatermark, the first
> > > >> condition
> > > >> > > > > > > > > > > > of DelayedOperationPurgatory will be satisfied
> > > >> > > immediately
> > > >> > > > > when
> > > >> > > > > > > > > broker
> > > >> > > > > > > > > > > > receives PurgeRequest. Broker will send
> > > >> PurgeResponse
> > > >> > to
> > > >> > > > > admin
> > > >> > > > > > > > client
> > > >> > > > > > > > > > > > immediately. The response maps this partition
> to
> > > the
> > > >> > > > > > > lowWatermark.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > This case is covered as the first condition of
> > > >> > > > > > > > > > DelayedOperationPurgatory
> > > >> > > > > > > > > > > in
> > > >> > > > > > > > > > > > the current KIP.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > b) If highWatermark < offsetToPurge <
> > > logEndOffset,
> > > >> > > leader
> > > >> > > > > will
> > > >> > > > > > > > send
> > > >> > > > > > > > > > > > FetchResponse with
> low_watermark=offsetToPurge.
> > > >> > Follower
> > > >> > > > > > records
> > > >> > > > > > > > the
> > > >> > > > > > > > > > > > offsetToPurge as low_watermark and sends
> > > >> FetchRequest
> > > >> > to
> > > >> > > > the
> > > >> > > > > > > leader
> > > >> > > > > > > > > > with
> > > >> > > > > > > > > > > > the new low_watermark. Leader will then send
> > > >> > > PurgeResponse
> > > >> > > > to
> > > >> > > > > > > admin
> > > >> > > > > > > > > > > client
> > > >> > > > > > > > > > > > which maps this partition to the new
> > > low_watermark.
> > > >> The
> > > >> > > > data
> > > >> > > > > in
> > > >> > > > > > > the
> > > >> > > > > > > > > > range
> > > >> > > > > > > > > > > > [highWatermark, offsetToPurge] will still be
> > > >> appended
> > > >> > > from
> > > >> > > > > > leader
> > > >> > > > > > > > to
> > > >> > > > > > > > > > > > followers but will not be exposed to
> consumers.
> > > And
> > > >> in
> > > >> > a
> > > >> > > > > short
> > > >> > > > > > > > period
> > > >> > > > > > > > > > of
> > > >> > > > > > > > > > > > time low_watermark on the follower will be
> > higher
> > > >> than
> > > >> > > > their
> > > >> > > > > > > > > > > highWatermark.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > This case is also covered in the current KIP
> so
> > no
> > > >> > change
> > > >> > > > is
> > > >> > > > > > > > > required.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > c) If logEndOffset < offsetToPurge, leader
> will
> > > send
> > > >> > > > > > > PurgeResponse
> > > >> > > > > > > > to
> > > >> > > > > > > > > > > admin
> > > >> > > > > > > > > > > > client immediately. The response maps this
> > > >> partition to
> > > >> > > > > > > > > > > > OffsetOutOfRangeException.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > This case is not covered by the current KIP. I
> > > just
> > > >> > added
> > > >> > > > > this
> > > >> > > > > > as
> > > >> > > > > > > > the
> > > >> > > > > > > > > > > > second condition for the PurgeRequest to be
> > > removed
> > > >> > from
> > > >> > > > > > > > > > > > DelayedOperationPurgatory (in the Proposed
> > Change
> > > >> > > section).
> > > >> > > > > > Since
> > > >> > > > > > > > the
> > > >> > > > > > > > > > > > PurgeRequest is satisfied immediately when the
> > > >> leader
> > > >> > > > > receives
> > > >> > > > > > > it,
> > > >> > > > > > > > it
> > > >> > > > > > > > > > > > actually won't be put into the
> > > >> > DelayedOperationPurgatory.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > 3. Yes, lowWatermark will be used when
> > > >> smallest_offset
> > > >> > is
> > > >> > > > > used
> > > >> > > > > > in
> > > >> > > > > > > > the
> > > >> > > > > > > > > > > > ListOffsetRequest. I just updated Proposed
> > Change
> > > >> > section
> > > >> > > > to
> > > >> > > > > > > > specify
> > > >> > > > > > > > > > > this.
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > Thanks,
> > > >> > > > > > > > > > > > Dong
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > On Tue, Jan 17, 2017 at 6:53 PM, Jun Rao <
> > > >> > > jun@confluent.io
> > > >> > > > >
> > > >> > > > > > > wrote:
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Hi, Dong,
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Thanks for the KIP. Looks good overall.
> Just a
> > > few
> > > >> > more
> > > >> > > > > > > comments.
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > 1."Note that the way broker handles
> > PurgeRequest
> > > >> is
> > > >> > > > similar
> > > >> > > > > > to
> > > >> > > > > > > > how
> > > >> > > > > > > > > it
> > > >> > > > > > > > > > > > > handles ProduceRequest with ack = -1 and
> > > >> > > > isr=all_replicas".
> > > >> > > > > > It
> > > >> > > > > > > > > seems
> > > >> > > > > > > > > > > that
> > > >> > > > > > > > > > > > > the implementation is a bit different. In
> this
> > > >> KIP,
> > > >> > we
> > > >> > > > wait
> > > >> > > > > > for
> > > >> > > > > > > > all
> > > >> > > > > > > > > > > > > replicas. But in producer, acks=all means
> > > waiting
> > > >> for
> > > >> > > all
> > > >> > > > > > > in-sync
> > > >> > > > > > > > > > > > replicas.
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > 2. Could you describe the behavior when the
> > > >> specified
> > > >> > > > > > > > offsetToPurge
> > > >> > > > > > > > > > is
> > > >> > > > > > > > > > > > (a)
> > > >> > > > > > > > > > > > > smaller than lowWatermark, (b) larger than
> > > >> > > highWatermark,
> > > >> > > > > but
> > > >> > > > > > > > > smaller
> > > >> > > > > > > > > > > > than
> > > >> > > > > > > > > > > > > log end offset, (c) larger than log end
> > offset?
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > 3. In the ListOffsetRequest, will
> lowWatermark
> > > be
> > > >> > > > returned
> > > >> > > > > > when
> > > >> > > > > > > > the
> > > >> > > > > > > > > > > > > smallest_offset option is used?
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Thanks,
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > Jun
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > On Wed, Jan 11, 2017 at 1:01 PM, Dong Lin <
> > > >> > > > > > lindong28@gmail.com
> > > >> > > > > > > >
> > > >> > > > > > > > > > wrote:
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > Hi all,
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > It seems that there is no further concern
> > with
> > > >> the
> > > >> > > > > KIP-107.
> > > >> > > > > > > At
> > > >> > > > > > > > > this
> > > >> > > > > > > > > > > > point
> > > >> > > > > > > > > > > > > > we would like to start the voting process.
> > The
> > > >> KIP
> > > >> > > can
> > > >> > > > be
> > > >> > > > > > > found
> > > >> > > > > > > > > at
> > > >> > > > > > > > > > > > > > https://cwiki.apache.org/confl
> > > >> > > uence/display/KAFKA/KIP-
> > > >> > > > > 107
> > > >> > > > > > > > > > > > > > %3A+Add+purgeDataBefore%28%29+
> > > >> API+in+AdminClient.
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > > > Thanks,
> > > >> > > > > > > > > > > > > > Dong
> > > >> > > > > > > > > > > > > >
> > > >> > > > > > > > > > > > >
> > > >> > > > > > > > > > > >
> > > >> > > > > > > > > > >
> > > >> > > > > > > > > >
> > > >> > > > > > > > >
> > > >> > > > > > > >
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > >
> > > >> > > > > > > --
> > > >> > > > > > > -- Guozhang
> > > >> > > > > > >
> > > >> > > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > > >
> > > >> > > > > --
> > > >> > > > > -- Guozhang
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> > >
> > > >> > >
> > > >> > > --
> > > >> > > -- Guozhang
> > > >> > >
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>

--f403045e6ff80cd84805476e3ddf--