Hey Jun,

I have been thinking about whether it is better to return lag (i.e. HW -
LEO) instead of LEO. Note that the lag in the DescribeDirsResponse may be
negative if LEO > HW. It will almost always be negative for leader and
in-sync replicas. Note that we can not calculate lag as max(0, HW - LEO)
because we still need the difference between two lags to measure the
progress of intra-broker replica movement. The AdminClient API can choose
to return max(0, HW - LEO) depending on whether it is used for tracking
progress of inter-broker reassignment or intra-broker movement. Is it OK?
If so, I will update the KIP-113 accordingly to return lag in the
DescribeDirsResponse .

Thanks,
Dong



<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon>
Virus-free.
www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link>
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

On Wed, Aug 9, 2017 at 5:06 PM, Jun Rao <j...@confluent.io> wrote:

> Hi, Dong,
>
> Yes, the lag in a replica is calculated as the difference of LEO of the
> replica and the HW. So, as long as a replica is in sync, the lag is always
> 0.
>
> So, I was suggesting to return lag instead of LEO in DescribeDirsResponse
> for each replica. I am not sure if we need to return HW though.
>
> Thanks,
>
> Jun
>
> On Wed, Aug 9, 2017 at 5:01 PM, Dong Lin <lindon...@gmail.com> wrote:
>
> > Hey Jun,
> >
> > It just came to me that you may be assuming that folower_lag = HW -
> > follower_LEO. If that is the case, then we need to have new
> > request/response to retrieve this lag since the DescribeDirsResponse
> > doesn't even include HW. It seems that KIP-179 does not explicitly
> specify
> > the definition of this lag.
> >
> > I have been assuming that follow_lag = leader_LEO - follower_LEO given
> that
> > the request is used to query the reassignment status. Strictly speaking
> the
> > difference between leader_LEO and the HW is limited by the amount of data
> > produced in KafkaConfig.replicaLagTimeMaxMs, which is 10 seconds. I also
> > assumed that 10 seconds is probably not a big deal given the typical time
> > length of the reassignment.
> >
> > Thanks,
> > Dong
> >
> > On Wed, Aug 9, 2017 at 4:40 PM, Dong Lin <lindon...@gmail.com> wrote:
> >
> > > Hey Jun,
> > >
> > > If I understand you right, you are suggesting that, in the case when
> > there
> > > is continuous incoming traffic, the approach in the KIP-179 will report
> > lag
> > > as 0 whereas the approach using DescribeDirsRequest will report lag as
> > > non-zero. But I think the approach in KIP-179 will also report non-zero
> > lag
> > > when there is continuous traffic. This is because at the time the
> leader
> > > receives ReplicaStatusRequest, it is likely that some data has been
> > > appended to the partition after the last FetchRequest from the
> follower.
> > > Does this make sense?
> > >
> > > Thanks,
> > > Dong
> > >
> > >
> > >
> > > On Wed, Aug 9, 2017 at 4:24 PM, Jun Rao <j...@confluent.io> wrote:
> > >
> > >> Hi, Dong,
> > >>
> > >> As for whether to return LEO or lag, my point was the following. What
> > you
> > >> are concerned about is that an in-sync replica could become out of
> sync
> > >> again. However, the more common case is that once a replica is caught
> > up,
> > >> it will stay in sync afterwards. In that case, once the reassignment
> > >> process completes, if we report based on lag, all lags will be 0. If
> we
> > >> report based on Math.max(0, leaderLEO - followerLEO), the value may
> not
> > be
> > >> 0 if there is continuous incoming traffic, which will be confusing to
> > the
> > >> user.
> > >>
> > >> Thanks,
> > >>
> > >> Jun
> > >>
> > >>
> > >>
> > >> On Tue, Aug 8, 2017 at 6:26 PM, Dong Lin <lindon...@gmail.com> wrote:
> > >>
> > >> > Hey Jun,
> > >> >
> > >> > Thanks for the comment!
> > >> >
> > >> > Yes, it should work. The tool can send request to any broker and
> > broker
> > >> can
> > >> > just write the reassignment znode. My previous intuition is that it
> > may
> > >> be
> > >> > better to only send this request to controller. But I don't have
> good
> > >> > reasons for this restriction.
> > >> >
> > >> > My intuition is that we can keep them separate as well. Becket and I
> > >> have
> > >> > discussed this both offline and in https://github.com/apache/
> > >> > kafka/pull/3621.
> > >> > Currently I don't have a strong opinion on this and I am open to
> using
> > >> only
> > >> > one API to do both if someone can come up with a reasonable API
> > >> signature
> > >> > for this method. For now I have added the method alterReplicaDir()
> in
> > >> > KafkaAdminClient instead of the AdminClient interface so that the
> > >> > reassignment script can use this method without concluding what the
> > API
> > >> > would look like in AdminClient in the future.
> > >> >
> > >> > Regarding DescribeDirsResponse, I think it is probably OK to have
> > >> slightly
> > >> > more lag. The script can calculate the lag of the follower replica
> as
> > >> > Math.max(0, leaderLEO - followerLEO). I agree that it will be
> slightly
> > >> less
> > >> > accurate than the current approach in KIP-179. But even with the
> > current
> > >> > approach in KIP-179, the result provided by the script is an
> > >> approximation
> > >> > anyway, since there is delay from the time that leader returns
> > response
> > >> to
> > >> > the time that the script collects response from all brokers and
> prints
> > >> > result to user. I think if the slight difference in the accuracy
> > between
> > >> > the two approaches does not make a difference to the intended
> use-case
> > >> of
> > >> > this API, then we probably want to re-use the exiting
> request/response
> > >> to
> > >> > keep the protocol simple.
> > >> >
> > >> > Thanks,
> > >> > Dong
> > >> >
> > >> >
> > >> >
> > >> >
> > >> >
> > >> > On Tue, Aug 8, 2017 at 5:56 PM, Jun Rao <j...@confluent.io> wrote:
> > >> >
> > >> > > Hi, Dong,
> > >> > >
> > >> > > I think Tom was suggesting to have the AlterTopicsRequest sent to
> > any
> > >> > > broker, which just writes the reassignment json to ZK. The
> > controller
> > >> > will
> > >> > > pick up the reassignment and act on it as usual. This should work,
> > >> right?
> > >> > >
> > >> > > Having a separate AlterTopicsRequest and AlterReplicaDirRequest
> > seems
> > >> > > simpler to me. The former is handled by the controller and the
> > latter
> > >> is
> > >> > > handled by the affected broker. They don't always have to be done
> > >> > together.
> > >> > > Merging the two into a single request probably will make both the
> > api
> > >> and
> > >> > > the implementation a bit more complicated. If we do keep the two
> > >> separate
> > >> > > requests, it seems that we should just add AlterReplicaDirRequest
> to
> > >> the
> > >> > > AdminClient interface?
> > >> > >
> > >> > > Now, regarding DescribeDirsResponse. I agree that it can be used
> for
> > >> the
> > >> > > status reporting in KIP-179 as well. However, it seems that
> > reporting
> > >> the
> > >> > > log end offset of each replica may not be easy to use. The log end
> > >> offset
> > >> > > will be returned from different brokers in slightly different
> time.
> > If
> > >> > > there is continuous producing traffic, the difference in log end
> > >> offset
> > >> > > between the leader and the follower could be larger than 0 even if
> > the
> > >> > > follower has fully caught up. I am wondering if it's better to
> > instead
> > >> > > return the lag in offset per replica. This way, the status can
> > >> probably
> > >> > be
> > >> > > reported more reliably.
> > >> > >
> > >> > > Thanks,
> > >> > >
> > >> > > Jun
> > >> > >
> > >> > > On Tue, Aug 8, 2017 at 11:23 AM, Dong Lin <lindon...@gmail.com>
> > >> wrote:
> > >> > >
> > >> > > > Hey Tom,
> > >> > > >
> > >> > > > Thanks for the quick reply. Please see my comment inline.
> > >> > > >
> > >> > > > On Tue, Aug 8, 2017 at 11:06 AM, Tom Bentley <
> > t.j.bent...@gmail.com
> > >> >
> > >> > > > wrote:
> > >> > > >
> > >> > > > > Hi Dong,
> > >> > > > >
> > >> > > > > Replies inline, as usual
> > >> > > > >
> > >> > > > > > As I originally envisaged it, KIP-179's support for
> > reassigning
> > >> > > > > partitions
> > >> > > > > >
> > >> > > > > > would have more-or-less taken the logic currently in the
> > >> > > > > > > ReassignPartitionsCommand (that is, writing JSON to the
> > >> > > > > > > ZkUtils.ReassignPartitionsPath)
> > >> > > > > > > and put it behind a suitable network protocol API. Thus it
> > >> > wouldn't
> > >> > > > > > matter
> > >> > > > > > > which broker received the protocol call: It would be acted
> > on
> > >> by
> > >> > > > > brokers
> > >> > > > > > > being notified of the change in the ZK path, just as
> > >> currently.
> > >> > > This
> > >> > > > > > would
> > >> > > > > > > have kept the ReassignPartitionsCommand relatively simple,
> > as
> > >> it
> > >> > > > > > currently
> > >> > > > > > > is.
> > >> > > > > > >
> > >> > > > > >
> > >> > > > > > I am not sure I fully understand your proposal. I think you
> > are
> > >> > > saying
> > >> > > > > that
> > >> > > > > > any broker can receive and handle the AlterTopicRequest.
> > >> > > > >
> > >> > > > >
> > >> > > > > That's right.
> > >> > > > >
> > >> > > > >
> > >> > > > > > Let's say a
> > >> > > > > > non-controller broker received AlterTopicRequest, is this
> > broker
> > >> > > going
> > >> > > > to
> > >> > > > > > send LeaderAndIsrRequest to other brokers? Or is this broker
> > >> create
> > >> > > the
> > >> > > > > > reassignment znode in zookeper?
> > >> > > > >
> > >> > > > >
> > >> > > > > Exactly: It's going to write some JSON to the relevant znode.
> > >> Other
> > >> > > > brokers
> > >> > > > > will get notified by zk when the contents of this znode
> changes,
> > >> and
> > >> > do
> > >> > > > as
> > >> > > > > they do now. This is what the tool/script does now.
> > >> > > > >
> > >> > > > > I will confess that I don't completely understand the role of
> > >> > > > > LeaderAndIsrRequest, since the current code just seems to
> write
> > to
> > >> > the
> > >> > > > > znode do get the brokers to do the reassignment. If you could
> > >> explain
> > >> > > the
> > >> > > > > role of LeaderAndIsrRequest that would be great.
> > >> > > > >
> > >> > > >
> > >> > > > Currently only the controller will listen to the reassignment
> > znode
> > >> and
> > >> > > > sends LeaderAndIsrRequest and StopReplicaRequest to brokers in
> > >> order to
> > >> > > > complete reassignment. Brokers won't need to listen to zookeeper
> > for
> > >> > any
> > >> > > > reassignment -- brokers only reacts to the request from
> > controller.
> > >> > > > Currently Kafka's design replies a lot on the controller to
> keep a
> > >> > > > consistent view of who are the leader of partitions and what is
> > the
> > >> ISR
> > >> > > > etc. It will be a pretty drastic change, if not impossible, for
> > the
> > >> > > script
> > >> > > > to reassign partitions without going through controller.
> > >> > > >
> > >> > > > Thus I think it is likely that your AlterTopicsRequest can only
> be
> > >> sent
> > >> > > to
> > >> > > > controller. Then the controller can create the reassignment
> znode
> > in
> > >> > > > zookeeper so that the information is persisted across controller
> > >> fail
> > >> > > over.
> > >> > > > I haven't think through this in detail though.
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > >
> > >> > > > >
> > >> > > > > > I may have missed it. But I couldn't find
> > >> > > > > > the explanation of AlterTopicRequest handling in KIP-179.
> > >> > > > > >
> > >> > > > >
> > >> > > > > You're right, it doesn't go into that much detail. I will fix
> > >> that.
> > >> > > > >
> > >> > > > >
> > >> > > > > > >
> > >> > > > > > > KIP-113 is obviously seeking to make more radical changes.
> > The
> > >> > > > > algorithm
> > >> > > > > > > described for moving a replica to a particular directory
> on
> > a
> > >> > > > different
> > >> > > > > > > broker (
> > >> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> > > > > > > 113%3A+Support+replicas+movement+between+log+
> > >> > directories#KIP-113:
> > >> > > > > > > Supportreplicasmovementbetweenlogdirectories-2)
> > >> > > > > > > Howtoreassignreplicabetweenlogdirectoriesacrossbrokers
> > >> > > > > > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> > > > > > > 113%3A+Support+replicas+movement+between+log+
> > >> > directories#KIP-113:
> > >> > > > > > > Supportreplicasmovementbetweenlogdirectories-2%
> > >> > > > > > > 29Howtoreassignreplicabetweenl
> ogdirectoriesacrossbrokers>)
> > >> > > > > > > involves both sending AlterReplicaDirRequest to "the"
> broker
> > >> (the
> > >> > > > > > receiving
> > >> > > > > > > broker, I assume, but it's not spelled out), _as well as_
> > >> writing
> > >> > > to
> > >> > > > > the
> > >> > > > > > ZK
> > >> > > > > > > node.
> > >> > > > > > >
> > >> > > > > > > This assumes the script (ReassignPartitionsCommand) has
> > direct
> > >> > > access
> > >> > > > > to
> > >> > > > > > > ZooKeeper, which is what KIP-179 is seeking to deprecate.
> It
> > >> > seems
> > >> > > a
> > >> > > > > > waste
> > >> > > > > > > of time to put the logic in the script as part of KIP-113,
> > >> only
> > >> > for
> > >> > > > > > KIP-179
> > >> > > > > > > to have to move it back to the controller.
> > >> > > > > > >
> > >> > > > > >
> > >> > > > > > I am not sure I understand what you mean by "It seems a
> waste
> > of
> > >> > time
> > >> > > > to
> > >> > > > > > put the logic in the script as part of KIP-113, only for
> > >> KIP-179 to
> > >> > > > have
> > >> > > > > to
> > >> > > > > > move it back to the controller".
> > >> > > > >
> > >> > > > >
> > >> > > > > Sorry, I misunderstood slightly what you were proposing in
> > >> KIP-113,
> > >> > so
> > >> > > > the
> > >> > > > > "waste of time" comment isn't quite right, but I'm still not
> > >> > convinced
> > >> > > > that
> > >> > > > > KIP-113+KIP-179 (in its current form) ends with a satisfactory
> > >> > result.
> > >> > > > >
> > >> > > > > Let me elaborate... KIP-113 says that to support reassigning
> > >> replica
> > >> > > > > between log directories across brokers:
> > >> > > > > * ...
> > >> > > > > * The script sends AlterReplicaDirRequest to those brokers
> which
> > >> need
> > >> > > to
> > >> > > > > move replicas...
> > >> > > > > * The script creates reassignment znode in zookeeper.
> > >> > > > > * The script retries AlterReplicaDirRequest to those broker...
> > >> > > > > * ...
> > >> > > > >
> > >> > > > > So the ReassignPartitionsCommand still talks to ZK directly,
> but
> > >> now
> > >> > > it's
> > >> > > > > bracketed by calls to the AdminClient. KIP-179 could replace
> > that
> > >> > > talking
> > >> > > > > to ZK directly with a new call to the AdminClient. But then
> > we've
> > >> > got a
> > >> > > > > pretty weird API, where we have to make three AdminClient
> calls
> > >> (two
> > >> > of
> > >> > > > > them to the same method), to move a replica. I don't really
> > >> > understand
> > >> > > > why
> > >> > > > > the admin client can't present a single API method to achieve
> > >> this,
> > >> > and
> > >> > > > > encapsulate on the server side the careful sequence of events
> > >> > necessary
> > >> > > > to
> > >> > > > > coordinate the movement. I understood this position is what
> > Ismael
> > >> > was
> > >> > > > > advocating when he said it was better to put the logic in the
> > >> > > controller
> > >> > > > > than spread between the script and the controller. But maybe I
> > >> > > > > misunderstood him.
> > >> > > > >
> > >> > > >
> > >> > > > I have some concern with putting this logic in controller which
> > can
> > >> be
> > >> > > > found in my previous email. Before that is addressed, the script
> > (or
> > >> > > > AdminClient) seems to be the simplest place to have this logic.
> > >> > > >
> > >> > > > I agree it is better to have a single API to achieve both
> > partition
> > >> and
> > >> > > > replica -> dir assignment. I think it is likely that we will
> find
> > a
> > >> > good
> > >> > > > API to do both. I have updated the KIP-113 to remove API
> > >> > alterReplicaDir
> > >> > > > from AdminClient interface.
> > >> > > >
> > >> > > >
> > >> > > > >
> > >> > > > >
> > >> > > > > > I assume that the logic you mentioned is
> > >> > > > > > "movement of replica to the specified log directory". This
> > logic
> > >> > (or
> > >> > > > the
> > >> > > > > > implementation of this logic) resides mainly in the
> > >> > KafkaAdminClient
> > >> > > > and
> > >> > > > > > broker. The script only needs to parse the json file as
> > >> appropriate
> > >> > > and
> > >> > > > > > call the new API in AdminClient as appropriate. The logic in
> > the
> > >> > > script
> > >> > > > > is
> > >> > > > > > therefore not much and can be easily moved to other classes
> if
> > >> > > needed.
> > >> > > > > >
> > >> > > > > > Can you clarify why this logic, i.e. movement of replica to
> > the
> > >> > > > specified
> > >> > > > > > log directory, needs to be moved to controller in KIP-179? I
> > >> think
> > >> > it
> > >> > > > can
> > >> > > > > > still be done in the script and controller should not need
> to
> > >> worry
> > >> > > > about
> > >> > > > > > log directory of any replica.
> > >> > > > > >
> > >> > > > > > Thanks,
> > >> > > > > > Dong
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>

Reply via email to