You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jiangjie Qin <jq...@linkedin.com.INVALID> on 2015/08/07 18:29:54 UTC
[DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Hi,
I just created KIP-29 to add a new configuration of IsrPropagateIntervalMs
to KafkaConfig. The motivation is to batch and throttle the ISR propagation
in cases where large amount of ISR change occurs such as a broker gets
bounced or failed. Otherwise it might overwhelm the controller.
https://cwiki.apache.org/confluence/display/KAFKA/KIP-29+-+Add+an+IsrPropagateIntervalMs+configuration+to+KafkaConfig
This is a config addition and is backward compatible.
Thoughts?
Thanks,
Jiangjie (Becket) Qin
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Jiangjie Qin <jq...@linkedin.com.INVALID>.
Hi Jay,
I'm not sure I understand the "group commit". Can you elaborate a little
bit?
The ISR change are actually on a per partition base. When a broker fails
and come back, there could be thousands of ISR changes in say 30 seconds.
In that case how would you group them? In terms of ISR shrinking, it is
already sort of grouped because ISR shrinking check runs periodically. Not
sure how ISR expansion would work here.
Another approach might be just only use ISR shrinking check to propagate
ISR change, so we basically reuse replica.lag.time.max.ms. But I feel
reusing configuration usually causes more confusion than adding a new one.
Thanks,
Jiangjie (Becket) QIn
On Fri, Aug 7, 2015 at 9:52 AM, Jay Kreps <ja...@confluent.io> wrote:
> Would it be possible to take a "group commit" approach rather than
> configuring a delay. I.e. all changes that come in at once while another
> operation is occurring automatically get batched. This can sometimes be
> nicer and more automatic than another tunable parameter...
>
> -Jay
>
> On Fri, Aug 7, 2015 at 9:29 AM, Jiangjie Qin <jq...@linkedin.com.invalid>
> wrote:
>
> > Hi,
> >
> > I just created KIP-29 to add a new configuration of
> IsrPropagateIntervalMs
> > to KafkaConfig. The motivation is to batch and throttle the ISR
> propagation
> > in cases where large amount of ISR change occurs such as a broker gets
> > bounced or failed. Otherwise it might overwhelm the controller.
> >
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-29+-+Add+an+IsrPropagateIntervalMs+configuration+to+KafkaConfig
> >
> > This is a config addition and is backward compatible.
> >
> > Thoughts?
> >
> > Thanks,
> >
> > Jiangjie (Becket) Qin
> >
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Jay Kreps <ja...@confluent.io>.
Would it be possible to take a "group commit" approach rather than
configuring a delay. I.e. all changes that come in at once while another
operation is occurring automatically get batched. This can sometimes be
nicer and more automatic than another tunable parameter...
-Jay
On Fri, Aug 7, 2015 at 9:29 AM, Jiangjie Qin <jq...@linkedin.com.invalid>
wrote:
> Hi,
>
> I just created KIP-29 to add a new configuration of IsrPropagateIntervalMs
> to KafkaConfig. The motivation is to batch and throttle the ISR propagation
> in cases where large amount of ISR change occurs such as a broker gets
> bounced or failed. Otherwise it might overwhelm the controller.
>
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-29+-+Add+an+IsrPropagateIntervalMs+configuration+to+KafkaConfig
>
> This is a config addition and is backward compatible.
>
> Thoughts?
>
> Thanks,
>
> Jiangjie (Becket) Qin
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Jiangjie Qin <jq...@linkedin.com.INVALID>.
Jay,
Agreed that we should avoid such configurations if possible. Besides that I
think we should also try to avoid super complicated implementation that can
potentially cause even more problems.
Ashish,
For (1), it is basically the group commit Jay recommended. The
implementation might be a little bit tricky, though. What if broker 2
received a UpdateMetadataRequest which was triggered by broker 1?
For (2), the worst case propagation delay is still y seconds. Is it
essentially different from hard code the propagation interval to y seconds?
The goal we want to achieve here are:
1. If there are small number of ISR changes, we want them to propagate
immediately.
2. If there are large number of ISR changes, we want to batch and throttle
them.
Maybe we can do the following:
1. At time T0, if an ISR change occurs, we plan to send the ISR change at T
+ 100ms.
2. If no more ISR change occurs between T0 and T0+100ms, we send the ISR
notification. If a new ISR change occur at T1 (T0 < T1 < T0+100ms), we plan
to send at T1+100ms. So every newly added ISR change delay the send a
little bit.
3. If current time is at T0 + 1000ms, we always send the ISR notification.
In most cases, an ISR change will be propagated within 100ms during small
number of ISR change. and during peak we batch every second. There might be
small chance that an ISR change occur every 100 ms and we end up sending 10
notification / second. But that perhaps isn't too bad.
Thoughts?
Jiangjie (Becket) Qin
On Mon, Aug 10, 2015 at 10:15 AM, Ashish Singh <as...@cloudera.com> wrote:
> Not having to deal with configs sounds ideal. I liked the idea of having
> automatic batching, however that requires broker-to-broker comm support. At
> some point, I think it will be better to have direct broker - controller
> communication, rather that using ZK for the purpose.
>
> Few more approaches.
>
> 1. A broker that initiated IISR change notification will not issue another
> notification until it receives a metadata upgrade request from controller.
> At which point it can discard all the ISR change notifications it has
> queued.
> 2. ISR change notifications can be limited at each broker by rate rather
> than time. So, each broker can send up to something like x ISR change
> notifications in y seconds.
>
> On Mon, Aug 10, 2015 at 9:39 AM, Jay Kreps <ja...@confluent.io> wrote:
>
> > I guess the question, which I think is what Gwen was getting at, is if,
> > rather than making this configurable, it might be possible to make this
> > just work reliably and with the lowest possible latency in some automatic
> > fashion? I raised group commit because that is a way to automatically
> batch
> > under load. If that doesn't work perhaps there is another way? The
> > challenge as we've seen with obscure configs is that 99% of people can't
> > figure out how to set them so for 99% of people this won't really help
> > them.
> >
> > -Jay
> >
> > On Mon, Aug 10, 2015 at 9:12 AM, Ashish Singh <as...@cloudera.com>
> wrote:
> >
> > > Hey Guys,
> > >
> > > Looks like Jun and Jiangjie have covered the motivation behind the
> > config.
> > > To me one would want to set ISR propagation delay to a high number
> > > primarily during rolling upgrade. I think the delay one would want to
> > have
> > > in ISR propagation is proportional to the cluster size. However, during
> > > normal operations a faster ISR propagation is desired. Having a config
> to
> > > expose the delay time provides admin a way to control it, and it will
> > come
> > > with a default value so if someone does not want to play with it they
> can
> > > choose not to.
> > >
> > > @Gwen, does that answer your question?
> > >
> > > On Sun, Aug 9, 2015 at 3:26 PM, Jiangjie Qin <jqin@linkedin.com.invalid
> >
> > > wrote:
> > >
> > > > Jun,
> > > >
> > > > Thanks for the detailed explanation. Now I understand. Yes, we might
> > not
> > > be
> > > > able leverage the group commit in this case.
> > > >
> > > > When I was testing the patch, I also found a potential use case for
> the
> > > > config (not sure if that is a strong use case though). When we
> rolling
> > > > upgrade a cluster, if the controller is still running on an old
> > version,
> > > > the brokers got bounced will create a bunch of ZK path that will not
> be
> > > > picked up by the controller until the controller is upgraded to the
> new
> > > > version. It might be fine for small clusters, for larger clusters,
> > there
> > > > could be many ISR change got accumulated before the controller runs
> on
> > > the
> > > > new version. So if we have the config, when people do rolling
> upgrade,
> > > they
> > > > can first have the propagation interval to be very large so ISR
> change
> > > will
> > > > not be propagated. After all the brokers are running on the new
> > version,
> > > we
> > > > can set the propagation interval back to a short value and bounce
> > brokers
> > > > again.
> > > >
> > > > Thanks,
> > > >
> > > > Jiangjie (Becket) Qin
> > > >
> > > > On Sun, Aug 9, 2015 at 12:22 PM, Jun Rao <ju...@confluent.io> wrote:
> > > >
> > > > > Jiangjie,
> > > > >
> > > > > Related to group commit, I think what Jay says is the following.
> > > Suppose
> > > > > the propagation of ISR is slow and also creates back pressure, you
> > can
> > > > just
> > > > > propagate the ISR one batch at a time as fast as you can. After one
> > > batch
> > > > > is sent, you just collect all ISR changes that have accumulated
> since
> > > the
> > > > > last propagation and propagate them as a batch. This way, the
> amount
> > of
> > > > > batching happens automatically based on the propagation rate and
> the
> > > > delay
> > > > > of propagation is minimized. However, this doesn't quite work if we
> > > > > propagate the ISR changes by writing a notification path in ZK.
> There
> > > is
> > > > no
> > > > > back pressure: the notification can be written faster than the
> > listener
> > > > can
> > > > > process it. So the explicit batching that you did essentially is to
> > get
> > > > > around this problem.
> > > > >
> > > > > As for the config, I can see a couple of use cases: (1) We have
> unit
> > > > tests
> > > > > that verify the propagation of ISR. To prevent those tests taking
> too
> > > > long,
> > > > > we can configure a smaller propagation interval. (2) When there are
> > > many
> > > > > partitions and brokers, someone may want to increase the
> propagation
> > > > > interval to reduce the ZK overhead. I agree that most people likely
> > > don't
> > > > > need to change this setting since the default should be good
> enough.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jun
> > > > >
> > > > >
> > > > >
> > > > > On Fri, Aug 7, 2015 at 4:06 PM, Gwen Shapira <gw...@confluent.io>
> > > wrote:
> > > > >
> > > > > > Maybe Ashish can supply the use-case and tuning advice then :)
> > > > > > I'm a -1 on adding new configurations that we can't quite
> explain.
> > > > > >
> > > > > > On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin
> > > <jqin@linkedin.com.invalid
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Gwen,
> > > > > > >
> > > > > > > Completely agree with you. I originally just hard coded it to
> be
> > 10
> > > > > > > seconds. Ashish raised this requirement in KAFKA-2406 because
> > > people
> > > > > > might
> > > > > > > want to ISR changes get propagated quicker.
> > > > > > > I don't have a good use case myself. Personally I think hard
> code
> > > it
> > > > is
> > > > > > > fine although I don't object to make it configurable.
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > > Jiangjie (Becket) Qin
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > >
> > > Regards,
> > > Ashish
> > >
> >
>
>
>
> --
>
> Regards,
> Ashish
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Ashish Singh <as...@cloudera.com>.
Not having to deal with configs sounds ideal. I liked the idea of having
automatic batching, however that requires broker-to-broker comm support. At
some point, I think it will be better to have direct broker - controller
communication, rather that using ZK for the purpose.
Few more approaches.
1. A broker that initiated IISR change notification will not issue another
notification until it receives a metadata upgrade request from controller.
At which point it can discard all the ISR change notifications it has
queued.
2. ISR change notifications can be limited at each broker by rate rather
than time. So, each broker can send up to something like x ISR change
notifications in y seconds.
On Mon, Aug 10, 2015 at 9:39 AM, Jay Kreps <ja...@confluent.io> wrote:
> I guess the question, which I think is what Gwen was getting at, is if,
> rather than making this configurable, it might be possible to make this
> just work reliably and with the lowest possible latency in some automatic
> fashion? I raised group commit because that is a way to automatically batch
> under load. If that doesn't work perhaps there is another way? The
> challenge as we've seen with obscure configs is that 99% of people can't
> figure out how to set them so for 99% of people this won't really help
> them.
>
> -Jay
>
> On Mon, Aug 10, 2015 at 9:12 AM, Ashish Singh <as...@cloudera.com> wrote:
>
> > Hey Guys,
> >
> > Looks like Jun and Jiangjie have covered the motivation behind the
> config.
> > To me one would want to set ISR propagation delay to a high number
> > primarily during rolling upgrade. I think the delay one would want to
> have
> > in ISR propagation is proportional to the cluster size. However, during
> > normal operations a faster ISR propagation is desired. Having a config to
> > expose the delay time provides admin a way to control it, and it will
> come
> > with a default value so if someone does not want to play with it they can
> > choose not to.
> >
> > @Gwen, does that answer your question?
> >
> > On Sun, Aug 9, 2015 at 3:26 PM, Jiangjie Qin <jq...@linkedin.com.invalid>
> > wrote:
> >
> > > Jun,
> > >
> > > Thanks for the detailed explanation. Now I understand. Yes, we might
> not
> > be
> > > able leverage the group commit in this case.
> > >
> > > When I was testing the patch, I also found a potential use case for the
> > > config (not sure if that is a strong use case though). When we rolling
> > > upgrade a cluster, if the controller is still running on an old
> version,
> > > the brokers got bounced will create a bunch of ZK path that will not be
> > > picked up by the controller until the controller is upgraded to the new
> > > version. It might be fine for small clusters, for larger clusters,
> there
> > > could be many ISR change got accumulated before the controller runs on
> > the
> > > new version. So if we have the config, when people do rolling upgrade,
> > they
> > > can first have the propagation interval to be very large so ISR change
> > will
> > > not be propagated. After all the brokers are running on the new
> version,
> > we
> > > can set the propagation interval back to a short value and bounce
> brokers
> > > again.
> > >
> > > Thanks,
> > >
> > > Jiangjie (Becket) Qin
> > >
> > > On Sun, Aug 9, 2015 at 12:22 PM, Jun Rao <ju...@confluent.io> wrote:
> > >
> > > > Jiangjie,
> > > >
> > > > Related to group commit, I think what Jay says is the following.
> > Suppose
> > > > the propagation of ISR is slow and also creates back pressure, you
> can
> > > just
> > > > propagate the ISR one batch at a time as fast as you can. After one
> > batch
> > > > is sent, you just collect all ISR changes that have accumulated since
> > the
> > > > last propagation and propagate them as a batch. This way, the amount
> of
> > > > batching happens automatically based on the propagation rate and the
> > > delay
> > > > of propagation is minimized. However, this doesn't quite work if we
> > > > propagate the ISR changes by writing a notification path in ZK. There
> > is
> > > no
> > > > back pressure: the notification can be written faster than the
> listener
> > > can
> > > > process it. So the explicit batching that you did essentially is to
> get
> > > > around this problem.
> > > >
> > > > As for the config, I can see a couple of use cases: (1) We have unit
> > > tests
> > > > that verify the propagation of ISR. To prevent those tests taking too
> > > long,
> > > > we can configure a smaller propagation interval. (2) When there are
> > many
> > > > partitions and brokers, someone may want to increase the propagation
> > > > interval to reduce the ZK overhead. I agree that most people likely
> > don't
> > > > need to change this setting since the default should be good enough.
> > > >
> > > > Thanks,
> > > >
> > > > Jun
> > > >
> > > >
> > > >
> > > > On Fri, Aug 7, 2015 at 4:06 PM, Gwen Shapira <gw...@confluent.io>
> > wrote:
> > > >
> > > > > Maybe Ashish can supply the use-case and tuning advice then :)
> > > > > I'm a -1 on adding new configurations that we can't quite explain.
> > > > >
> > > > > On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin
> > <jqin@linkedin.com.invalid
> > > >
> > > > > wrote:
> > > > >
> > > > > > Hi Gwen,
> > > > > >
> > > > > > Completely agree with you. I originally just hard coded it to be
> 10
> > > > > > seconds. Ashish raised this requirement in KAFKA-2406 because
> > people
> > > > > might
> > > > > > want to ISR changes get propagated quicker.
> > > > > > I don't have a good use case myself. Personally I think hard code
> > it
> > > is
> > > > > > fine although I don't object to make it configurable.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Jiangjie (Becket) Qin
> > > > > >
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> >
> > Regards,
> > Ashish
> >
>
--
Regards,
Ashish
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Gwen Shapira <gw...@confluent.io>.
An easier (although less clean) solution can be a "hidden" configuration:
Avoid adding the new configuration to KafkaConfig's ConfigDef and the docs,
but grab a pre-defined parameter anyway (if exists, use the reasonable
default if it doesn't). This will allow us to override the value in tests
(and emergencies) but avoid confusing users with new user-exposed
configurations that they most likely won't need.
(The concept is inspired by Oracle's hidden parameters, which serve the
same goal but also has support implications)
Gwen
On Mon, Aug 10, 2015 at 9:39 AM, Jay Kreps <ja...@confluent.io> wrote:
> I guess the question, which I think is what Gwen was getting at, is if,
> rather than making this configurable, it might be possible to make this
> just work reliably and with the lowest possible latency in some automatic
> fashion? I raised group commit because that is a way to automatically batch
> under load. If that doesn't work perhaps there is another way? The
> challenge as we've seen with obscure configs is that 99% of people can't
> figure out how to set them so for 99% of people this won't really help
> them.
>
> -Jay
>
> On Mon, Aug 10, 2015 at 9:12 AM, Ashish Singh <as...@cloudera.com> wrote:
>
> > Hey Guys,
> >
> > Looks like Jun and Jiangjie have covered the motivation behind the
> config.
> > To me one would want to set ISR propagation delay to a high number
> > primarily during rolling upgrade. I think the delay one would want to
> have
> > in ISR propagation is proportional to the cluster size. However, during
> > normal operations a faster ISR propagation is desired. Having a config to
> > expose the delay time provides admin a way to control it, and it will
> come
> > with a default value so if someone does not want to play with it they can
> > choose not to.
> >
> > @Gwen, does that answer your question?
> >
> > On Sun, Aug 9, 2015 at 3:26 PM, Jiangjie Qin <jq...@linkedin.com.invalid>
> > wrote:
> >
> > > Jun,
> > >
> > > Thanks for the detailed explanation. Now I understand. Yes, we might
> not
> > be
> > > able leverage the group commit in this case.
> > >
> > > When I was testing the patch, I also found a potential use case for the
> > > config (not sure if that is a strong use case though). When we rolling
> > > upgrade a cluster, if the controller is still running on an old
> version,
> > > the brokers got bounced will create a bunch of ZK path that will not be
> > > picked up by the controller until the controller is upgraded to the new
> > > version. It might be fine for small clusters, for larger clusters,
> there
> > > could be many ISR change got accumulated before the controller runs on
> > the
> > > new version. So if we have the config, when people do rolling upgrade,
> > they
> > > can first have the propagation interval to be very large so ISR change
> > will
> > > not be propagated. After all the brokers are running on the new
> version,
> > we
> > > can set the propagation interval back to a short value and bounce
> brokers
> > > again.
> > >
> > > Thanks,
> > >
> > > Jiangjie (Becket) Qin
> > >
> > > On Sun, Aug 9, 2015 at 12:22 PM, Jun Rao <ju...@confluent.io> wrote:
> > >
> > > > Jiangjie,
> > > >
> > > > Related to group commit, I think what Jay says is the following.
> > Suppose
> > > > the propagation of ISR is slow and also creates back pressure, you
> can
> > > just
> > > > propagate the ISR one batch at a time as fast as you can. After one
> > batch
> > > > is sent, you just collect all ISR changes that have accumulated since
> > the
> > > > last propagation and propagate them as a batch. This way, the amount
> of
> > > > batching happens automatically based on the propagation rate and the
> > > delay
> > > > of propagation is minimized. However, this doesn't quite work if we
> > > > propagate the ISR changes by writing a notification path in ZK. There
> > is
> > > no
> > > > back pressure: the notification can be written faster than the
> listener
> > > can
> > > > process it. So the explicit batching that you did essentially is to
> get
> > > > around this problem.
> > > >
> > > > As for the config, I can see a couple of use cases: (1) We have unit
> > > tests
> > > > that verify the propagation of ISR. To prevent those tests taking too
> > > long,
> > > > we can configure a smaller propagation interval. (2) When there are
> > many
> > > > partitions and brokers, someone may want to increase the propagation
> > > > interval to reduce the ZK overhead. I agree that most people likely
> > don't
> > > > need to change this setting since the default should be good enough.
> > > >
> > > > Thanks,
> > > >
> > > > Jun
> > > >
> > > >
> > > >
> > > > On Fri, Aug 7, 2015 at 4:06 PM, Gwen Shapira <gw...@confluent.io>
> > wrote:
> > > >
> > > > > Maybe Ashish can supply the use-case and tuning advice then :)
> > > > > I'm a -1 on adding new configurations that we can't quite explain.
> > > > >
> > > > > On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin
> > <jqin@linkedin.com.invalid
> > > >
> > > > > wrote:
> > > > >
> > > > > > Hi Gwen,
> > > > > >
> > > > > > Completely agree with you. I originally just hard coded it to be
> 10
> > > > > > seconds. Ashish raised this requirement in KAFKA-2406 because
> > people
> > > > > might
> > > > > > want to ISR changes get propagated quicker.
> > > > > > I don't have a good use case myself. Personally I think hard code
> > it
> > > is
> > > > > > fine although I don't object to make it configurable.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Jiangjie (Becket) Qin
> > > > > >
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> >
> > Regards,
> > Ashish
> >
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Jay Kreps <ja...@confluent.io>.
I guess the question, which I think is what Gwen was getting at, is if,
rather than making this configurable, it might be possible to make this
just work reliably and with the lowest possible latency in some automatic
fashion? I raised group commit because that is a way to automatically batch
under load. If that doesn't work perhaps there is another way? The
challenge as we've seen with obscure configs is that 99% of people can't
figure out how to set them so for 99% of people this won't really help them.
-Jay
On Mon, Aug 10, 2015 at 9:12 AM, Ashish Singh <as...@cloudera.com> wrote:
> Hey Guys,
>
> Looks like Jun and Jiangjie have covered the motivation behind the config.
> To me one would want to set ISR propagation delay to a high number
> primarily during rolling upgrade. I think the delay one would want to have
> in ISR propagation is proportional to the cluster size. However, during
> normal operations a faster ISR propagation is desired. Having a config to
> expose the delay time provides admin a way to control it, and it will come
> with a default value so if someone does not want to play with it they can
> choose not to.
>
> @Gwen, does that answer your question?
>
> On Sun, Aug 9, 2015 at 3:26 PM, Jiangjie Qin <jq...@linkedin.com.invalid>
> wrote:
>
> > Jun,
> >
> > Thanks for the detailed explanation. Now I understand. Yes, we might not
> be
> > able leverage the group commit in this case.
> >
> > When I was testing the patch, I also found a potential use case for the
> > config (not sure if that is a strong use case though). When we rolling
> > upgrade a cluster, if the controller is still running on an old version,
> > the brokers got bounced will create a bunch of ZK path that will not be
> > picked up by the controller until the controller is upgraded to the new
> > version. It might be fine for small clusters, for larger clusters, there
> > could be many ISR change got accumulated before the controller runs on
> the
> > new version. So if we have the config, when people do rolling upgrade,
> they
> > can first have the propagation interval to be very large so ISR change
> will
> > not be propagated. After all the brokers are running on the new version,
> we
> > can set the propagation interval back to a short value and bounce brokers
> > again.
> >
> > Thanks,
> >
> > Jiangjie (Becket) Qin
> >
> > On Sun, Aug 9, 2015 at 12:22 PM, Jun Rao <ju...@confluent.io> wrote:
> >
> > > Jiangjie,
> > >
> > > Related to group commit, I think what Jay says is the following.
> Suppose
> > > the propagation of ISR is slow and also creates back pressure, you can
> > just
> > > propagate the ISR one batch at a time as fast as you can. After one
> batch
> > > is sent, you just collect all ISR changes that have accumulated since
> the
> > > last propagation and propagate them as a batch. This way, the amount of
> > > batching happens automatically based on the propagation rate and the
> > delay
> > > of propagation is minimized. However, this doesn't quite work if we
> > > propagate the ISR changes by writing a notification path in ZK. There
> is
> > no
> > > back pressure: the notification can be written faster than the listener
> > can
> > > process it. So the explicit batching that you did essentially is to get
> > > around this problem.
> > >
> > > As for the config, I can see a couple of use cases: (1) We have unit
> > tests
> > > that verify the propagation of ISR. To prevent those tests taking too
> > long,
> > > we can configure a smaller propagation interval. (2) When there are
> many
> > > partitions and brokers, someone may want to increase the propagation
> > > interval to reduce the ZK overhead. I agree that most people likely
> don't
> > > need to change this setting since the default should be good enough.
> > >
> > > Thanks,
> > >
> > > Jun
> > >
> > >
> > >
> > > On Fri, Aug 7, 2015 at 4:06 PM, Gwen Shapira <gw...@confluent.io>
> wrote:
> > >
> > > > Maybe Ashish can supply the use-case and tuning advice then :)
> > > > I'm a -1 on adding new configurations that we can't quite explain.
> > > >
> > > > On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin
> <jqin@linkedin.com.invalid
> > >
> > > > wrote:
> > > >
> > > > > Hi Gwen,
> > > > >
> > > > > Completely agree with you. I originally just hard coded it to be 10
> > > > > seconds. Ashish raised this requirement in KAFKA-2406 because
> people
> > > > might
> > > > > want to ISR changes get propagated quicker.
> > > > > I don't have a good use case myself. Personally I think hard code
> it
> > is
> > > > > fine although I don't object to make it configurable.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Jiangjie (Becket) Qin
> > > > >
> > > >
> > >
> >
>
>
>
> --
>
> Regards,
> Ashish
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Ashish Singh <as...@cloudera.com>.
Hey Guys,
Looks like Jun and Jiangjie have covered the motivation behind the config.
To me one would want to set ISR propagation delay to a high number
primarily during rolling upgrade. I think the delay one would want to have
in ISR propagation is proportional to the cluster size. However, during
normal operations a faster ISR propagation is desired. Having a config to
expose the delay time provides admin a way to control it, and it will come
with a default value so if someone does not want to play with it they can
choose not to.
@Gwen, does that answer your question?
On Sun, Aug 9, 2015 at 3:26 PM, Jiangjie Qin <jq...@linkedin.com.invalid>
wrote:
> Jun,
>
> Thanks for the detailed explanation. Now I understand. Yes, we might not be
> able leverage the group commit in this case.
>
> When I was testing the patch, I also found a potential use case for the
> config (not sure if that is a strong use case though). When we rolling
> upgrade a cluster, if the controller is still running on an old version,
> the brokers got bounced will create a bunch of ZK path that will not be
> picked up by the controller until the controller is upgraded to the new
> version. It might be fine for small clusters, for larger clusters, there
> could be many ISR change got accumulated before the controller runs on the
> new version. So if we have the config, when people do rolling upgrade, they
> can first have the propagation interval to be very large so ISR change will
> not be propagated. After all the brokers are running on the new version, we
> can set the propagation interval back to a short value and bounce brokers
> again.
>
> Thanks,
>
> Jiangjie (Becket) Qin
>
> On Sun, Aug 9, 2015 at 12:22 PM, Jun Rao <ju...@confluent.io> wrote:
>
> > Jiangjie,
> >
> > Related to group commit, I think what Jay says is the following. Suppose
> > the propagation of ISR is slow and also creates back pressure, you can
> just
> > propagate the ISR one batch at a time as fast as you can. After one batch
> > is sent, you just collect all ISR changes that have accumulated since the
> > last propagation and propagate them as a batch. This way, the amount of
> > batching happens automatically based on the propagation rate and the
> delay
> > of propagation is minimized. However, this doesn't quite work if we
> > propagate the ISR changes by writing a notification path in ZK. There is
> no
> > back pressure: the notification can be written faster than the listener
> can
> > process it. So the explicit batching that you did essentially is to get
> > around this problem.
> >
> > As for the config, I can see a couple of use cases: (1) We have unit
> tests
> > that verify the propagation of ISR. To prevent those tests taking too
> long,
> > we can configure a smaller propagation interval. (2) When there are many
> > partitions and brokers, someone may want to increase the propagation
> > interval to reduce the ZK overhead. I agree that most people likely don't
> > need to change this setting since the default should be good enough.
> >
> > Thanks,
> >
> > Jun
> >
> >
> >
> > On Fri, Aug 7, 2015 at 4:06 PM, Gwen Shapira <gw...@confluent.io> wrote:
> >
> > > Maybe Ashish can supply the use-case and tuning advice then :)
> > > I'm a -1 on adding new configurations that we can't quite explain.
> > >
> > > On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin <jqin@linkedin.com.invalid
> >
> > > wrote:
> > >
> > > > Hi Gwen,
> > > >
> > > > Completely agree with you. I originally just hard coded it to be 10
> > > > seconds. Ashish raised this requirement in KAFKA-2406 because people
> > > might
> > > > want to ISR changes get propagated quicker.
> > > > I don't have a good use case myself. Personally I think hard code it
> is
> > > > fine although I don't object to make it configurable.
> > > >
> > > > Thanks,
> > > >
> > > > Jiangjie (Becket) Qin
> > > >
> > >
> >
>
--
Regards,
Ashish
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Jiangjie Qin <jq...@linkedin.com.INVALID>.
Jun,
Thanks for the detailed explanation. Now I understand. Yes, we might not be
able leverage the group commit in this case.
When I was testing the patch, I also found a potential use case for the
config (not sure if that is a strong use case though). When we rolling
upgrade a cluster, if the controller is still running on an old version,
the brokers got bounced will create a bunch of ZK path that will not be
picked up by the controller until the controller is upgraded to the new
version. It might be fine for small clusters, for larger clusters, there
could be many ISR change got accumulated before the controller runs on the
new version. So if we have the config, when people do rolling upgrade, they
can first have the propagation interval to be very large so ISR change will
not be propagated. After all the brokers are running on the new version, we
can set the propagation interval back to a short value and bounce brokers
again.
Thanks,
Jiangjie (Becket) Qin
On Sun, Aug 9, 2015 at 12:22 PM, Jun Rao <ju...@confluent.io> wrote:
> Jiangjie,
>
> Related to group commit, I think what Jay says is the following. Suppose
> the propagation of ISR is slow and also creates back pressure, you can just
> propagate the ISR one batch at a time as fast as you can. After one batch
> is sent, you just collect all ISR changes that have accumulated since the
> last propagation and propagate them as a batch. This way, the amount of
> batching happens automatically based on the propagation rate and the delay
> of propagation is minimized. However, this doesn't quite work if we
> propagate the ISR changes by writing a notification path in ZK. There is no
> back pressure: the notification can be written faster than the listener can
> process it. So the explicit batching that you did essentially is to get
> around this problem.
>
> As for the config, I can see a couple of use cases: (1) We have unit tests
> that verify the propagation of ISR. To prevent those tests taking too long,
> we can configure a smaller propagation interval. (2) When there are many
> partitions and brokers, someone may want to increase the propagation
> interval to reduce the ZK overhead. I agree that most people likely don't
> need to change this setting since the default should be good enough.
>
> Thanks,
>
> Jun
>
>
>
> On Fri, Aug 7, 2015 at 4:06 PM, Gwen Shapira <gw...@confluent.io> wrote:
>
> > Maybe Ashish can supply the use-case and tuning advice then :)
> > I'm a -1 on adding new configurations that we can't quite explain.
> >
> > On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin <jq...@linkedin.com.invalid>
> > wrote:
> >
> > > Hi Gwen,
> > >
> > > Completely agree with you. I originally just hard coded it to be 10
> > > seconds. Ashish raised this requirement in KAFKA-2406 because people
> > might
> > > want to ISR changes get propagated quicker.
> > > I don't have a good use case myself. Personally I think hard code it is
> > > fine although I don't object to make it configurable.
> > >
> > > Thanks,
> > >
> > > Jiangjie (Becket) Qin
> > >
> >
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Jun Rao <ju...@confluent.io>.
Jiangjie,
Related to group commit, I think what Jay says is the following. Suppose
the propagation of ISR is slow and also creates back pressure, you can just
propagate the ISR one batch at a time as fast as you can. After one batch
is sent, you just collect all ISR changes that have accumulated since the
last propagation and propagate them as a batch. This way, the amount of
batching happens automatically based on the propagation rate and the delay
of propagation is minimized. However, this doesn't quite work if we
propagate the ISR changes by writing a notification path in ZK. There is no
back pressure: the notification can be written faster than the listener can
process it. So the explicit batching that you did essentially is to get
around this problem.
As for the config, I can see a couple of use cases: (1) We have unit tests
that verify the propagation of ISR. To prevent those tests taking too long,
we can configure a smaller propagation interval. (2) When there are many
partitions and brokers, someone may want to increase the propagation
interval to reduce the ZK overhead. I agree that most people likely don't
need to change this setting since the default should be good enough.
Thanks,
Jun
On Fri, Aug 7, 2015 at 4:06 PM, Gwen Shapira <gw...@confluent.io> wrote:
> Maybe Ashish can supply the use-case and tuning advice then :)
> I'm a -1 on adding new configurations that we can't quite explain.
>
> On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin <jq...@linkedin.com.invalid>
> wrote:
>
> > Hi Gwen,
> >
> > Completely agree with you. I originally just hard coded it to be 10
> > seconds. Ashish raised this requirement in KAFKA-2406 because people
> might
> > want to ISR changes get propagated quicker.
> > I don't have a good use case myself. Personally I think hard code it is
> > fine although I don't object to make it configurable.
> >
> > Thanks,
> >
> > Jiangjie (Becket) Qin
> >
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Gwen Shapira <gw...@confluent.io>.
Maybe Ashish can supply the use-case and tuning advice then :)
I'm a -1 on adding new configurations that we can't quite explain.
On Fri, Aug 7, 2015 at 3:57 PM, Jiangjie Qin <jq...@linkedin.com.invalid>
wrote:
> Hi Gwen,
>
> Completely agree with you. I originally just hard coded it to be 10
> seconds. Ashish raised this requirement in KAFKA-2406 because people might
> want to ISR changes get propagated quicker.
> I don't have a good use case myself. Personally I think hard code it is
> fine although I don't object to make it configurable.
>
> Thanks,
>
> Jiangjie (Becket) Qin
>
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Jiangjie Qin <jq...@linkedin.com.INVALID>.
Hi Gwen,
Completely agree with you. I originally just hard coded it to be 10
seconds. Ashish raised this requirement in KAFKA-2406 because people might
want to ISR changes get propagated quicker.
I don't have a good use case myself. Personally I think hard code it is
fine although I don't object to make it configurable.
Thanks,
Jiangjie (Becket) Qin
Re: [DISCUSSION] KIP-29 - Add an IsrPropagateIntervalMs config to KafkaConfig
Posted by Gwen Shapira <gw...@confluent.io>.
Can you give some information on when I may want to tune this parameter?
I'm wary of overwhelming admins with options and not giving any reasonable
story on what they can / will achieve when modifying them.
On Fri, Aug 7, 2015 at 9:29 AM, Jiangjie Qin <jq...@linkedin.com.invalid>
wrote:
> Hi,
>
> I just created KIP-29 to add a new configuration of IsrPropagateIntervalMs
> to KafkaConfig. The motivation is to batch and throttle the ISR propagation
> in cases where large amount of ISR change occurs such as a broker gets
> bounced or failed. Otherwise it might overwhelm the controller.
>
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-29+-+Add+an+IsrPropagateIntervalMs+configuration+to+KafkaConfig
>
> This is a config addition and is backward compatible.
>
> Thoughts?
>
> Thanks,
>
> Jiangjie (Becket) Qin
>