You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Colin McCabe <cm...@apache.org> on 2018/01/19 02:15:27 UTC
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Hi all,
I updated the KIP. There is also an implementation of this KIP here: https://github.com/apache/kafka/pull/4418
The updated implementation simplifies a few things, and adds the ability to incrementally add or remove individual partitions in an incremental fetch request.
best,
Colin
On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> Hi all,
>
> I'd like to start the vote on KIP-227: Incremental Fetch Requests.
>
> The KIP is here:
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-227%3A+Introduce+Incremental+FetchRequests+to+Increase+Partition+Scalability
>
> and discussion thread earlier:
> https://www.mail-archive.com/dev@kafka.apache.org/msg83115.html
>
> thanks,
> Colin
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Colin McCabe <cm...@apache.org>.
On Thu, Jan 25, 2018, at 16:10, Colin McCabe wrote:
> On Thu, Jan 25, 2018, at 02:23, Mickael Maison wrote:
> > I'm late to the party but +1 and thanks for the KIP
>
> Thanks
>
> >
> > On Thu, Jan 25, 2018 at 12:36 AM, Ismael Juma <is...@juma.me.uk> wrote:
> > > Agreed, Jun.
> > >
> > > Ismael
> > >
> > > On Wed, Jan 24, 2018 at 4:08 PM, Jun Rao <ju...@confluent.io> wrote:
> > >
> > >> Since this is a server side metric, it's probably better to use Yammer Rate
> > >> (which has count) for consistency.
>
> Good point.
I updated the KIP to have a IncrementalFetchSessionEvictionsPerSec: as suggested here.
Also, I added a table of contents at the beginning, and updated the FetchRequest Metadata table.
thanks,
Colin
>
> best,
> Colin
>
> > >>
> > >> Thanks,
> > >>
> > >> Jun
> > >>
> > >> On Tue, Jan 23, 2018 at 10:17 PM, Colin McCabe <co...@cmccabe.xyz> wrote:
> > >>
> > >> > On Tue, Jan 23, 2018, at 21:47, Ismael Juma wrote:
> > >> > > Colin,
> > >> > >
> > >> > > You get a cumulative count for rates since we added
> > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> > 187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics>
> > >> >
> > >> > Oh, good point.
> > >> >
> > >> >
> > >> >
> > >> >
> > >> > > Ismael
> > >> > >
> > >> > > On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe
> > >> > > <cm...@apache.org> wrote:>
> > >> > > > On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> > >> > > > > Hi, Collin,
> > >> > > > >
> > >> > > > > Thanks for the updated KIP. +1. Just a minor comment. It seems
> > >> > > > > that it's> > > better for TotalIncrementalFetchSessionsEvicted to
> > >> > be a rate,
> > >> > > > > instead of> > > just an ever-growing count.
> > >> > > >
> > >> > > > Thanks. Perhaps we can add the rate in addition to the total
> > >> > > > eviction> > count?
> > >> > > >
> > >> > > > best,
> > >> > > > Colin
> > >> > > >
> > >> > > > >
> > >> > > > > Jun
> > >> > > > >
> > >> > > > > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson
> > >> > > > > <ja...@confluent.io>> > wrote:
> > >> > > > >
> > >> > > > > > >
> > >> > > > > > > What if we want to have fetch sessions for non-incremental
> > >> > > > > > > fetches> > in the
> > >> > > > > > > future, though? Also, we don't expect this configuration to
> > >> > > > > > > be> > changed
> > >> > > > > > > often, so it doesn't really need to be short.
> > >> > > > > >
> > >> > > > > >
> > >> > > > > > Hmm.. But in that case, I'm not sure we'd need to distinguish
> > >> > > > > > the two> > > > cases. If the non-incremental sessions are
> > >> > occupying space
> > >> > > > proportional to
> > >> > > > > > the fetched partitions, using the same config for both would be>
> > >> >
> > >> > reasonable.
> > >> > > > > > If they are not (which is more likely), we probably wouldn't
> > >> > > > > > need a> > config
> > >> > > > > > at all. Given that, I'd probably still opt for the more concise
> > >> > > > > > name.> > It's
> > >> > > > > > not a blocker for me though.
> > >> > > > > >
> > >> > > > > > +1 on the KIP.
> > >> > > > > >
> > >> > > > > > -Jason
> > >> > > > > >
> > >> > > > > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe
> > >> > > > > > <cm...@apache.org>> > wrote:
> > >> > > > > >
> > >> > > > > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > >> > > > > > > > Hi Colin,
> > >> > > > > > > >
> > >> > > > > > > > This is looking good to me. A few comments:
> > >> > > > > > > >
> > >> > > > > > > > 1. The fetch type seems unnecessary in the request and
> > >> > > > > > > > response> > schemas
> > >> > > > > > > > since it can be inferred by the sessionId/epoch.
> > >> > > > > > >
> > >> > > > > > > Hi Jason,
> > >> > > > > > >
> > >> > > > > > > Fair enough... if we need it later, we can always bump the RPC>
> > >> > > version.
> > >> > > > > > >
> > >> > > > > > > > 2. I agree with Jun that a separate array for partitions to
> > >> > > > > > > > remove> > > > would
> > >> > > > > > > be
> > >> > > > > > > > more intuitive.
> > >> > > > > > >
> > >> > > > > > > OK. I'll switch it to using a separate array.
> > >> > > > > > >
> > >> > > > > > > > 3. I'm not super thrilled with the cache configuration since
> > >> > > > > > > > it> > seems
> > >> > > > > > to
> > >> > > > > > > > tie us a bit too closely to the implementation. You've
> > >> > > > > > > > mostly> > convinced
> > >> > > > > > > me
> > >> > > > > > > > on the need for the slots config, but I wonder if we can at
> > >> > > > > > > > least> > do
> > >> > > > > > > > without "min.incremental.fetch.session.eviction.ms"? For
> > >> > > > > > > > one, I> > think
> > >> > > > > > > the
> > >> > > > > > > > broker should reserve the right to evict sessions at will.
> > >> > > > > > > > We> > shouldn't
> > >> > > > > > > be
> > >> > > > > > > > stuck maintaining a small session at the expense of a much
> > >> > > > > > > > larger> > one
> > >> > > > > > > just
> > >> > > > > > > > to enforce this timeout. Internally, I think having some
> > >> > > > > > > > cache> > > > stickiness
> > >> > > > > > > > to avoid thrashing makes sense, but I think static values
> > >> > > > > > > > are> > likely to
> > >> > > > > > > be
> > >> > > > > > > > good enough and that lets us retain some flexibility to
> > >> > > > > > > > change the> > > > > behavior
> > >> > > > > > > > in the future.
> > >> > > > > > >
> > >> > > > > > > OK.
> > >> > > > > > >
> > >> > > > > > > > 4. I think the word "incremental" is redundant in the config
> > >> > > > > > > > names.> > > > Maybe
> > >> > > > > > > > it could just be "max.fetch.session.cache.slots" for
> > >> > > > > > > > example?> > > > >
> > >> > > > > > > What if we want to have fetch sessions for non-incremental
> > >> > > > > > > fetches> > in the
> > >> > > > > > > future, though? Also, we don't expect this configuration to
> > >> > > > > > > be> > changed
> > >> > > > > > > often, so it doesn't really need to be short.
> > >> > > > > > >
> > >> > > > > > > best,
> > >> > > > > > > Colin
> > >> > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > Thanks,
> > >> > > > > > > > Jason
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > >
> > >> > > > > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe
> > >> > > > > > > > <cm...@apache.org> > >
> > >> > > > > > > wrote:
> > >> > > > > > > >
> > >> > > > > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > >> > > > > > > > > > Hi, Colin,
> > >> > > > > > > > > >
> > >> > > > > > > > > > Thanks for the KIP. Looks good to me overall. Just a
> > >> > > > > > > > > > couple of> > more
> > >> > > > > > > > > > comments.
> > >> > > > > > > > > >
> > >> > > > > > > > > > 1. As I mentioned earlier, it might be useful to add
> > >> > > > > > > > > > some> > metrics
> > >> > > > > > for
> > >> > > > > > > > > > monitoring the usage of the session cache. For example,
> > >> > > > > > > > > > it> > would be
> > >> > > > > > > > > useful
> > >> > > > > > > > > > to know how many slots are being used (or unused), # of
> > >> > > > > > > > > > total> > > > > partitions
> > >> > > > > > > > > in
> > >> > > > > > > > > > the cached slots (to understand space), the eviction
> > >> > > > > > > > > > rate (to> > see
> > >> > > > > > if
> > >> > > > > > > > > there
> > >> > > > > > > > > > is any churn), etc.
> > >> > > > > > > > >
> > >> > > > > > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but
> > >> > > > > > > > > I> > forgot
> > >> > > > > > > about
> > >> > > > > > > > > it. I just added some proposed metrics to the KIP wiki.
> > >> > > > > > > > >
> > >> > > > > > > > > >
> > >> > > > > > > > > > 2. Using max_bytes to 0 represent the removal of a
> > >> > > > > > > > > > partition> > seems
> > >> > > > > > > > > > unintuitive. Perhaps it's better to either add a flag
> > >> > > > > > > > > > per> > partition
> > >> > > > > > > or
> > >> > > > > > > > > add
> > >> > > > > > > > > > a removed partition list.
> > >> > > > > > > > >
> > >> > > > > > > > > Perhaps if we use max_bytes -1 to represent removal, it
> > >> > > > > > > > > will be> > more
> > >> > > > > > > > > intuitive? After all, -1 bytes is clearly not a valid
> > >> > > > > > > > > amount of> > > > bytes
> > >> > > > > > > to
> > >> > > > > > > > > fetch. Or should be have a separate array of removed
> > >> > > > > > TopicPartitions?
> > >> > > > > > > > >
> > >> > > > > > > > > On a related note, in the FetchResponse#PartitionData, we
> > >> > > > > > > > > have an> > > > > "error"
> > >> > > > > > > > > field, plus highWatermark, lastStableOffset,
> > >> > > > > > > > > logStartOffset, etc.> > > > But
> > >> > > > > > > when
> > >> > > > > > > > > the "error" field is set, those other fields are not used.>
> > >> > > Perhaps
> > >> > > > > > we
> > >> > > > > > > > > could save some space by just having a separate array of
> > >> > > > "partitions
> > >> > > > > > > with
> > >> > > > > > > > > errors." In the common case where there are no errors,
> > >> > > > > > > > > this> > would
> > >> > > > > > > save 2
> > >> > > > > > > > > bytes per partition, which could be quite significant in
> > >> > > > > > > > > large> > > > > responses.
> > >> > > > > > > > >
> > >> > > > > > > > > best,
> > >> > > > > > > > > Colin
> > >> > > > > > > > >
> > >> > > > > > > > > >
> > >> > > > > > > > > > Jun
> > >> > > > > > > > > >
> > >> > > > > > > > > >
> > >> > > > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
> > >> > > > cmccabe@apache.org>
> > >> > > > > > > > > wrote:
> > >> > > > > > > > > >
> > >> > > > > > > > > > > Hi all,
> > >> > > > > > > > > > >
> > >> > > > > > > > > > > I updated the KIP. There is also an implementation of
> > >> > > > > > > > > > > this> > KIP
> > >> > > > > > > here:
> > >> > > > > > > > > > > https://github.com/apache/kafka/pull/4418
> > >> > > > > > > > > > >
> > >> > > > > > > > > > > The updated implementation simplifies a few things,
> > >> > > > > > > > > > > and adds> > the
> > >> > > > > > > > > ability
> > >> > > > > > > > > > > to incrementally add or remove individual partitions
> > >> > > > > > > > > > > in an> > > > > incremental
> > >> > > > > > > > > > > fetch request.
> > >> > > > > > > > > > >
> > >> > > > > > > > > > > best,
> > >> > > > > > > > > > > Colin
> > >> > > > > > > > > > >
> > >> > > > > > > > > > >
> > >> > > > > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > >> > > > > > > > > > > > Hi all,
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > I'd like to start the vote on KIP-227: Incremental
> > >> > > > > > > > > > > > Fetch> > > > > Requests.
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > The KIP is here:
> > >> > > > > > > > > > > > https://cwiki.apache.org/
> > >> confluence/display/KAFKA/KIP->
> > >> > > > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increas-
> > >> > > > > > > > > > > e+> > > > > > > > > Partition+Scalability
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > and discussion thread earlier:
> > >> > > > > > > > > > > > https://www.mail-archive.com/
> > >> > dev@kafka.apache.org/msg83115> > .
> > >> > > > > > html
> > >> > > > > > > > > > > >
> > >> > > > > > > > > > > > thanks,
> > >> > > > > > > > > > > > Colin
> > >> > > > > > > > > > >
> > >> > > > > > > > >
> > >> > > > > > >
> > >> > > > > >
> > >> > > >
> > >> >
> > >> >
> > >>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Colin McCabe <cm...@apache.org>.
On Thu, Jan 25, 2018, at 02:23, Mickael Maison wrote:
> I'm late to the party but +1 and thanks for the KIP
Thanks
>
> On Thu, Jan 25, 2018 at 12:36 AM, Ismael Juma <is...@juma.me.uk> wrote:
> > Agreed, Jun.
> >
> > Ismael
> >
> > On Wed, Jan 24, 2018 at 4:08 PM, Jun Rao <ju...@confluent.io> wrote:
> >
> >> Since this is a server side metric, it's probably better to use Yammer Rate
> >> (which has count) for consistency.
Good point.
best,
Colin
> >>
> >> Thanks,
> >>
> >> Jun
> >>
> >> On Tue, Jan 23, 2018 at 10:17 PM, Colin McCabe <co...@cmccabe.xyz> wrote:
> >>
> >> > On Tue, Jan 23, 2018, at 21:47, Ismael Juma wrote:
> >> > > Colin,
> >> > >
> >> > > You get a cumulative count for rates since we added
> >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> > 187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics>
> >> >
> >> > Oh, good point.
> >> >
> >> >
> >> >
> >> >
> >> > > Ismael
> >> > >
> >> > > On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe
> >> > > <cm...@apache.org> wrote:>
> >> > > > On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> >> > > > > Hi, Collin,
> >> > > > >
> >> > > > > Thanks for the updated KIP. +1. Just a minor comment. It seems
> >> > > > > that it's> > > better for TotalIncrementalFetchSessionsEvicted to
> >> > be a rate,
> >> > > > > instead of> > > just an ever-growing count.
> >> > > >
> >> > > > Thanks. Perhaps we can add the rate in addition to the total
> >> > > > eviction> > count?
> >> > > >
> >> > > > best,
> >> > > > Colin
> >> > > >
> >> > > > >
> >> > > > > Jun
> >> > > > >
> >> > > > > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson
> >> > > > > <ja...@confluent.io>> > wrote:
> >> > > > >
> >> > > > > > >
> >> > > > > > > What if we want to have fetch sessions for non-incremental
> >> > > > > > > fetches> > in the
> >> > > > > > > future, though? Also, we don't expect this configuration to
> >> > > > > > > be> > changed
> >> > > > > > > often, so it doesn't really need to be short.
> >> > > > > >
> >> > > > > >
> >> > > > > > Hmm.. But in that case, I'm not sure we'd need to distinguish
> >> > > > > > the two> > > > cases. If the non-incremental sessions are
> >> > occupying space
> >> > > > proportional to
> >> > > > > > the fetched partitions, using the same config for both would be>
> >> >
> >> > reasonable.
> >> > > > > > If they are not (which is more likely), we probably wouldn't
> >> > > > > > need a> > config
> >> > > > > > at all. Given that, I'd probably still opt for the more concise
> >> > > > > > name.> > It's
> >> > > > > > not a blocker for me though.
> >> > > > > >
> >> > > > > > +1 on the KIP.
> >> > > > > >
> >> > > > > > -Jason
> >> > > > > >
> >> > > > > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe
> >> > > > > > <cm...@apache.org>> > wrote:
> >> > > > > >
> >> > > > > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> >> > > > > > > > Hi Colin,
> >> > > > > > > >
> >> > > > > > > > This is looking good to me. A few comments:
> >> > > > > > > >
> >> > > > > > > > 1. The fetch type seems unnecessary in the request and
> >> > > > > > > > response> > schemas
> >> > > > > > > > since it can be inferred by the sessionId/epoch.
> >> > > > > > >
> >> > > > > > > Hi Jason,
> >> > > > > > >
> >> > > > > > > Fair enough... if we need it later, we can always bump the RPC>
> >> > > version.
> >> > > > > > >
> >> > > > > > > > 2. I agree with Jun that a separate array for partitions to
> >> > > > > > > > remove> > > > would
> >> > > > > > > be
> >> > > > > > > > more intuitive.
> >> > > > > > >
> >> > > > > > > OK. I'll switch it to using a separate array.
> >> > > > > > >
> >> > > > > > > > 3. I'm not super thrilled with the cache configuration since
> >> > > > > > > > it> > seems
> >> > > > > > to
> >> > > > > > > > tie us a bit too closely to the implementation. You've
> >> > > > > > > > mostly> > convinced
> >> > > > > > > me
> >> > > > > > > > on the need for the slots config, but I wonder if we can at
> >> > > > > > > > least> > do
> >> > > > > > > > without "min.incremental.fetch.session.eviction.ms"? For
> >> > > > > > > > one, I> > think
> >> > > > > > > the
> >> > > > > > > > broker should reserve the right to evict sessions at will.
> >> > > > > > > > We> > shouldn't
> >> > > > > > > be
> >> > > > > > > > stuck maintaining a small session at the expense of a much
> >> > > > > > > > larger> > one
> >> > > > > > > just
> >> > > > > > > > to enforce this timeout. Internally, I think having some
> >> > > > > > > > cache> > > > stickiness
> >> > > > > > > > to avoid thrashing makes sense, but I think static values
> >> > > > > > > > are> > likely to
> >> > > > > > > be
> >> > > > > > > > good enough and that lets us retain some flexibility to
> >> > > > > > > > change the> > > > > behavior
> >> > > > > > > > in the future.
> >> > > > > > >
> >> > > > > > > OK.
> >> > > > > > >
> >> > > > > > > > 4. I think the word "incremental" is redundant in the config
> >> > > > > > > > names.> > > > Maybe
> >> > > > > > > > it could just be "max.fetch.session.cache.slots" for
> >> > > > > > > > example?> > > > >
> >> > > > > > > What if we want to have fetch sessions for non-incremental
> >> > > > > > > fetches> > in the
> >> > > > > > > future, though? Also, we don't expect this configuration to
> >> > > > > > > be> > changed
> >> > > > > > > often, so it doesn't really need to be short.
> >> > > > > > >
> >> > > > > > > best,
> >> > > > > > > Colin
> >> > > > > > >
> >> > > > > > > >
> >> > > > > > > > Thanks,
> >> > > > > > > > Jason
> >> > > > > > > >
> >> > > > > > > >
> >> > > > > > > >
> >> > > > > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe
> >> > > > > > > > <cm...@apache.org> > >
> >> > > > > > > wrote:
> >> > > > > > > >
> >> > > > > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> >> > > > > > > > > > Hi, Colin,
> >> > > > > > > > > >
> >> > > > > > > > > > Thanks for the KIP. Looks good to me overall. Just a
> >> > > > > > > > > > couple of> > more
> >> > > > > > > > > > comments.
> >> > > > > > > > > >
> >> > > > > > > > > > 1. As I mentioned earlier, it might be useful to add
> >> > > > > > > > > > some> > metrics
> >> > > > > > for
> >> > > > > > > > > > monitoring the usage of the session cache. For example,
> >> > > > > > > > > > it> > would be
> >> > > > > > > > > useful
> >> > > > > > > > > > to know how many slots are being used (or unused), # of
> >> > > > > > > > > > total> > > > > partitions
> >> > > > > > > > > in
> >> > > > > > > > > > the cached slots (to understand space), the eviction
> >> > > > > > > > > > rate (to> > see
> >> > > > > > if
> >> > > > > > > > > there
> >> > > > > > > > > > is any churn), etc.
> >> > > > > > > > >
> >> > > > > > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but
> >> > > > > > > > > I> > forgot
> >> > > > > > > about
> >> > > > > > > > > it. I just added some proposed metrics to the KIP wiki.
> >> > > > > > > > >
> >> > > > > > > > > >
> >> > > > > > > > > > 2. Using max_bytes to 0 represent the removal of a
> >> > > > > > > > > > partition> > seems
> >> > > > > > > > > > unintuitive. Perhaps it's better to either add a flag
> >> > > > > > > > > > per> > partition
> >> > > > > > > or
> >> > > > > > > > > add
> >> > > > > > > > > > a removed partition list.
> >> > > > > > > > >
> >> > > > > > > > > Perhaps if we use max_bytes -1 to represent removal, it
> >> > > > > > > > > will be> > more
> >> > > > > > > > > intuitive? After all, -1 bytes is clearly not a valid
> >> > > > > > > > > amount of> > > > bytes
> >> > > > > > > to
> >> > > > > > > > > fetch. Or should be have a separate array of removed
> >> > > > > > TopicPartitions?
> >> > > > > > > > >
> >> > > > > > > > > On a related note, in the FetchResponse#PartitionData, we
> >> > > > > > > > > have an> > > > > "error"
> >> > > > > > > > > field, plus highWatermark, lastStableOffset,
> >> > > > > > > > > logStartOffset, etc.> > > > But
> >> > > > > > > when
> >> > > > > > > > > the "error" field is set, those other fields are not used.>
> >> > > Perhaps
> >> > > > > > we
> >> > > > > > > > > could save some space by just having a separate array of
> >> > > > "partitions
> >> > > > > > > with
> >> > > > > > > > > errors." In the common case where there are no errors,
> >> > > > > > > > > this> > would
> >> > > > > > > save 2
> >> > > > > > > > > bytes per partition, which could be quite significant in
> >> > > > > > > > > large> > > > > responses.
> >> > > > > > > > >
> >> > > > > > > > > best,
> >> > > > > > > > > Colin
> >> > > > > > > > >
> >> > > > > > > > > >
> >> > > > > > > > > > Jun
> >> > > > > > > > > >
> >> > > > > > > > > >
> >> > > > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
> >> > > > cmccabe@apache.org>
> >> > > > > > > > > wrote:
> >> > > > > > > > > >
> >> > > > > > > > > > > Hi all,
> >> > > > > > > > > > >
> >> > > > > > > > > > > I updated the KIP. There is also an implementation of
> >> > > > > > > > > > > this> > KIP
> >> > > > > > > here:
> >> > > > > > > > > > > https://github.com/apache/kafka/pull/4418
> >> > > > > > > > > > >
> >> > > > > > > > > > > The updated implementation simplifies a few things,
> >> > > > > > > > > > > and adds> > the
> >> > > > > > > > > ability
> >> > > > > > > > > > > to incrementally add or remove individual partitions
> >> > > > > > > > > > > in an> > > > > incremental
> >> > > > > > > > > > > fetch request.
> >> > > > > > > > > > >
> >> > > > > > > > > > > best,
> >> > > > > > > > > > > Colin
> >> > > > > > > > > > >
> >> > > > > > > > > > >
> >> > > > > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> >> > > > > > > > > > > > Hi all,
> >> > > > > > > > > > > >
> >> > > > > > > > > > > > I'd like to start the vote on KIP-227: Incremental
> >> > > > > > > > > > > > Fetch> > > > > Requests.
> >> > > > > > > > > > > >
> >> > > > > > > > > > > > The KIP is here:
> >> > > > > > > > > > > > https://cwiki.apache.org/
> >> confluence/display/KAFKA/KIP->
> >> > > > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increas-
> >> > > > > > > > > > > e+> > > > > > > > > Partition+Scalability
> >> > > > > > > > > > > >
> >> > > > > > > > > > > > and discussion thread earlier:
> >> > > > > > > > > > > > https://www.mail-archive.com/
> >> > dev@kafka.apache.org/msg83115> > .
> >> > > > > > html
> >> > > > > > > > > > > >
> >> > > > > > > > > > > > thanks,
> >> > > > > > > > > > > > Colin
> >> > > > > > > > > > >
> >> > > > > > > > >
> >> > > > > > >
> >> > > > > >
> >> > > >
> >> >
> >> >
> >>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Mickael Maison <mi...@gmail.com>.
I'm late to the party but +1 and thanks for the KIP
On Thu, Jan 25, 2018 at 12:36 AM, Ismael Juma <is...@juma.me.uk> wrote:
> Agreed, Jun.
>
> Ismael
>
> On Wed, Jan 24, 2018 at 4:08 PM, Jun Rao <ju...@confluent.io> wrote:
>
>> Since this is a server side metric, it's probably better to use Yammer Rate
>> (which has count) for consistency.
>>
>> Thanks,
>>
>> Jun
>>
>> On Tue, Jan 23, 2018 at 10:17 PM, Colin McCabe <co...@cmccabe.xyz> wrote:
>>
>> > On Tue, Jan 23, 2018, at 21:47, Ismael Juma wrote:
>> > > Colin,
>> > >
>> > > You get a cumulative count for rates since we added
>> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> > 187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics>
>> >
>> > Oh, good point.
>> >
>> > C.
>> >
>> >
>> > > Ismael
>> > >
>> > > On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe
>> > > <cm...@apache.org> wrote:>
>> > > > On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
>> > > > > Hi, Collin,
>> > > > >
>> > > > > Thanks for the updated KIP. +1. Just a minor comment. It seems
>> > > > > that it's> > > better for TotalIncrementalFetchSessionsEvicted to
>> > be a rate,
>> > > > > instead of> > > just an ever-growing count.
>> > > >
>> > > > Thanks. Perhaps we can add the rate in addition to the total
>> > > > eviction> > count?
>> > > >
>> > > > best,
>> > > > Colin
>> > > >
>> > > > >
>> > > > > Jun
>> > > > >
>> > > > > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson
>> > > > > <ja...@confluent.io>> > wrote:
>> > > > >
>> > > > > > >
>> > > > > > > What if we want to have fetch sessions for non-incremental
>> > > > > > > fetches> > in the
>> > > > > > > future, though? Also, we don't expect this configuration to
>> > > > > > > be> > changed
>> > > > > > > often, so it doesn't really need to be short.
>> > > > > >
>> > > > > >
>> > > > > > Hmm.. But in that case, I'm not sure we'd need to distinguish
>> > > > > > the two> > > > cases. If the non-incremental sessions are
>> > occupying space
>> > > > proportional to
>> > > > > > the fetched partitions, using the same config for both would be>
>> >
>> > reasonable.
>> > > > > > If they are not (which is more likely), we probably wouldn't
>> > > > > > need a> > config
>> > > > > > at all. Given that, I'd probably still opt for the more concise
>> > > > > > name.> > It's
>> > > > > > not a blocker for me though.
>> > > > > >
>> > > > > > +1 on the KIP.
>> > > > > >
>> > > > > > -Jason
>> > > > > >
>> > > > > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe
>> > > > > > <cm...@apache.org>> > wrote:
>> > > > > >
>> > > > > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
>> > > > > > > > Hi Colin,
>> > > > > > > >
>> > > > > > > > This is looking good to me. A few comments:
>> > > > > > > >
>> > > > > > > > 1. The fetch type seems unnecessary in the request and
>> > > > > > > > response> > schemas
>> > > > > > > > since it can be inferred by the sessionId/epoch.
>> > > > > > >
>> > > > > > > Hi Jason,
>> > > > > > >
>> > > > > > > Fair enough... if we need it later, we can always bump the RPC>
>> > > version.
>> > > > > > >
>> > > > > > > > 2. I agree with Jun that a separate array for partitions to
>> > > > > > > > remove> > > > would
>> > > > > > > be
>> > > > > > > > more intuitive.
>> > > > > > >
>> > > > > > > OK. I'll switch it to using a separate array.
>> > > > > > >
>> > > > > > > > 3. I'm not super thrilled with the cache configuration since
>> > > > > > > > it> > seems
>> > > > > > to
>> > > > > > > > tie us a bit too closely to the implementation. You've
>> > > > > > > > mostly> > convinced
>> > > > > > > me
>> > > > > > > > on the need for the slots config, but I wonder if we can at
>> > > > > > > > least> > do
>> > > > > > > > without "min.incremental.fetch.session.eviction.ms"? For
>> > > > > > > > one, I> > think
>> > > > > > > the
>> > > > > > > > broker should reserve the right to evict sessions at will.
>> > > > > > > > We> > shouldn't
>> > > > > > > be
>> > > > > > > > stuck maintaining a small session at the expense of a much
>> > > > > > > > larger> > one
>> > > > > > > just
>> > > > > > > > to enforce this timeout. Internally, I think having some
>> > > > > > > > cache> > > > stickiness
>> > > > > > > > to avoid thrashing makes sense, but I think static values
>> > > > > > > > are> > likely to
>> > > > > > > be
>> > > > > > > > good enough and that lets us retain some flexibility to
>> > > > > > > > change the> > > > > behavior
>> > > > > > > > in the future.
>> > > > > > >
>> > > > > > > OK.
>> > > > > > >
>> > > > > > > > 4. I think the word "incremental" is redundant in the config
>> > > > > > > > names.> > > > Maybe
>> > > > > > > > it could just be "max.fetch.session.cache.slots" for
>> > > > > > > > example?> > > > >
>> > > > > > > What if we want to have fetch sessions for non-incremental
>> > > > > > > fetches> > in the
>> > > > > > > future, though? Also, we don't expect this configuration to
>> > > > > > > be> > changed
>> > > > > > > often, so it doesn't really need to be short.
>> > > > > > >
>> > > > > > > best,
>> > > > > > > Colin
>> > > > > > >
>> > > > > > > >
>> > > > > > > > Thanks,
>> > > > > > > > Jason
>> > > > > > > >
>> > > > > > > >
>> > > > > > > >
>> > > > > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe
>> > > > > > > > <cm...@apache.org> > >
>> > > > > > > wrote:
>> > > > > > > >
>> > > > > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
>> > > > > > > > > > Hi, Colin,
>> > > > > > > > > >
>> > > > > > > > > > Thanks for the KIP. Looks good to me overall. Just a
>> > > > > > > > > > couple of> > more
>> > > > > > > > > > comments.
>> > > > > > > > > >
>> > > > > > > > > > 1. As I mentioned earlier, it might be useful to add
>> > > > > > > > > > some> > metrics
>> > > > > > for
>> > > > > > > > > > monitoring the usage of the session cache. For example,
>> > > > > > > > > > it> > would be
>> > > > > > > > > useful
>> > > > > > > > > > to know how many slots are being used (or unused), # of
>> > > > > > > > > > total> > > > > partitions
>> > > > > > > > > in
>> > > > > > > > > > the cached slots (to understand space), the eviction
>> > > > > > > > > > rate (to> > see
>> > > > > > if
>> > > > > > > > > there
>> > > > > > > > > > is any churn), etc.
>> > > > > > > > >
>> > > > > > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but
>> > > > > > > > > I> > forgot
>> > > > > > > about
>> > > > > > > > > it. I just added some proposed metrics to the KIP wiki.
>> > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > > 2. Using max_bytes to 0 represent the removal of a
>> > > > > > > > > > partition> > seems
>> > > > > > > > > > unintuitive. Perhaps it's better to either add a flag
>> > > > > > > > > > per> > partition
>> > > > > > > or
>> > > > > > > > > add
>> > > > > > > > > > a removed partition list.
>> > > > > > > > >
>> > > > > > > > > Perhaps if we use max_bytes -1 to represent removal, it
>> > > > > > > > > will be> > more
>> > > > > > > > > intuitive? After all, -1 bytes is clearly not a valid
>> > > > > > > > > amount of> > > > bytes
>> > > > > > > to
>> > > > > > > > > fetch. Or should be have a separate array of removed
>> > > > > > TopicPartitions?
>> > > > > > > > >
>> > > > > > > > > On a related note, in the FetchResponse#PartitionData, we
>> > > > > > > > > have an> > > > > "error"
>> > > > > > > > > field, plus highWatermark, lastStableOffset,
>> > > > > > > > > logStartOffset, etc.> > > > But
>> > > > > > > when
>> > > > > > > > > the "error" field is set, those other fields are not used.>
>> > > Perhaps
>> > > > > > we
>> > > > > > > > > could save some space by just having a separate array of
>> > > > "partitions
>> > > > > > > with
>> > > > > > > > > errors." In the common case where there are no errors,
>> > > > > > > > > this> > would
>> > > > > > > save 2
>> > > > > > > > > bytes per partition, which could be quite significant in
>> > > > > > > > > large> > > > > responses.
>> > > > > > > > >
>> > > > > > > > > best,
>> > > > > > > > > Colin
>> > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > > Jun
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
>> > > > cmccabe@apache.org>
>> > > > > > > > > wrote:
>> > > > > > > > > >
>> > > > > > > > > > > Hi all,
>> > > > > > > > > > >
>> > > > > > > > > > > I updated the KIP. There is also an implementation of
>> > > > > > > > > > > this> > KIP
>> > > > > > > here:
>> > > > > > > > > > > https://github.com/apache/kafka/pull/4418
>> > > > > > > > > > >
>> > > > > > > > > > > The updated implementation simplifies a few things,
>> > > > > > > > > > > and adds> > the
>> > > > > > > > > ability
>> > > > > > > > > > > to incrementally add or remove individual partitions
>> > > > > > > > > > > in an> > > > > incremental
>> > > > > > > > > > > fetch request.
>> > > > > > > > > > >
>> > > > > > > > > > > best,
>> > > > > > > > > > > Colin
>> > > > > > > > > > >
>> > > > > > > > > > >
>> > > > > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
>> > > > > > > > > > > > Hi all,
>> > > > > > > > > > > >
>> > > > > > > > > > > > I'd like to start the vote on KIP-227: Incremental
>> > > > > > > > > > > > Fetch> > > > > Requests.
>> > > > > > > > > > > >
>> > > > > > > > > > > > The KIP is here:
>> > > > > > > > > > > > https://cwiki.apache.org/
>> confluence/display/KAFKA/KIP->
>> > > > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increas-
>> > > > > > > > > > > e+> > > > > > > > > Partition+Scalability
>> > > > > > > > > > > >
>> > > > > > > > > > > > and discussion thread earlier:
>> > > > > > > > > > > > https://www.mail-archive.com/
>> > dev@kafka.apache.org/msg83115> > .
>> > > > > > html
>> > > > > > > > > > > >
>> > > > > > > > > > > > thanks,
>> > > > > > > > > > > > Colin
>> > > > > > > > > > >
>> > > > > > > > >
>> > > > > > >
>> > > > > >
>> > > >
>> >
>> >
>>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Ismael Juma <is...@juma.me.uk>.
Agreed, Jun.
Ismael
On Wed, Jan 24, 2018 at 4:08 PM, Jun Rao <ju...@confluent.io> wrote:
> Since this is a server side metric, it's probably better to use Yammer Rate
> (which has count) for consistency.
>
> Thanks,
>
> Jun
>
> On Tue, Jan 23, 2018 at 10:17 PM, Colin McCabe <co...@cmccabe.xyz> wrote:
>
> > On Tue, Jan 23, 2018, at 21:47, Ismael Juma wrote:
> > > Colin,
> > >
> > > You get a cumulative count for rates since we added
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics>
> >
> > Oh, good point.
> >
> > C.
> >
> >
> > > Ismael
> > >
> > > On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe
> > > <cm...@apache.org> wrote:>
> > > > On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> > > > > Hi, Collin,
> > > > >
> > > > > Thanks for the updated KIP. +1. Just a minor comment. It seems
> > > > > that it's> > > better for TotalIncrementalFetchSessionsEvicted to
> > be a rate,
> > > > > instead of> > > just an ever-growing count.
> > > >
> > > > Thanks. Perhaps we can add the rate in addition to the total
> > > > eviction> > count?
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > > Jun
> > > > >
> > > > > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson
> > > > > <ja...@confluent.io>> > wrote:
> > > > >
> > > > > > >
> > > > > > > What if we want to have fetch sessions for non-incremental
> > > > > > > fetches> > in the
> > > > > > > future, though? Also, we don't expect this configuration to
> > > > > > > be> > changed
> > > > > > > often, so it doesn't really need to be short.
> > > > > >
> > > > > >
> > > > > > Hmm.. But in that case, I'm not sure we'd need to distinguish
> > > > > > the two> > > > cases. If the non-incremental sessions are
> > occupying space
> > > > proportional to
> > > > > > the fetched partitions, using the same config for both would be>
> >
> > reasonable.
> > > > > > If they are not (which is more likely), we probably wouldn't
> > > > > > need a> > config
> > > > > > at all. Given that, I'd probably still opt for the more concise
> > > > > > name.> > It's
> > > > > > not a blocker for me though.
> > > > > >
> > > > > > +1 on the KIP.
> > > > > >
> > > > > > -Jason
> > > > > >
> > > > > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe
> > > > > > <cm...@apache.org>> > wrote:
> > > > > >
> > > > > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > > > > > > > Hi Colin,
> > > > > > > >
> > > > > > > > This is looking good to me. A few comments:
> > > > > > > >
> > > > > > > > 1. The fetch type seems unnecessary in the request and
> > > > > > > > response> > schemas
> > > > > > > > since it can be inferred by the sessionId/epoch.
> > > > > > >
> > > > > > > Hi Jason,
> > > > > > >
> > > > > > > Fair enough... if we need it later, we can always bump the RPC>
> > > version.
> > > > > > >
> > > > > > > > 2. I agree with Jun that a separate array for partitions to
> > > > > > > > remove> > > > would
> > > > > > > be
> > > > > > > > more intuitive.
> > > > > > >
> > > > > > > OK. I'll switch it to using a separate array.
> > > > > > >
> > > > > > > > 3. I'm not super thrilled with the cache configuration since
> > > > > > > > it> > seems
> > > > > > to
> > > > > > > > tie us a bit too closely to the implementation. You've
> > > > > > > > mostly> > convinced
> > > > > > > me
> > > > > > > > on the need for the slots config, but I wonder if we can at
> > > > > > > > least> > do
> > > > > > > > without "min.incremental.fetch.session.eviction.ms"? For
> > > > > > > > one, I> > think
> > > > > > > the
> > > > > > > > broker should reserve the right to evict sessions at will.
> > > > > > > > We> > shouldn't
> > > > > > > be
> > > > > > > > stuck maintaining a small session at the expense of a much
> > > > > > > > larger> > one
> > > > > > > just
> > > > > > > > to enforce this timeout. Internally, I think having some
> > > > > > > > cache> > > > stickiness
> > > > > > > > to avoid thrashing makes sense, but I think static values
> > > > > > > > are> > likely to
> > > > > > > be
> > > > > > > > good enough and that lets us retain some flexibility to
> > > > > > > > change the> > > > > behavior
> > > > > > > > in the future.
> > > > > > >
> > > > > > > OK.
> > > > > > >
> > > > > > > > 4. I think the word "incremental" is redundant in the config
> > > > > > > > names.> > > > Maybe
> > > > > > > > it could just be "max.fetch.session.cache.slots" for
> > > > > > > > example?> > > > >
> > > > > > > What if we want to have fetch sessions for non-incremental
> > > > > > > fetches> > in the
> > > > > > > future, though? Also, we don't expect this configuration to
> > > > > > > be> > changed
> > > > > > > often, so it doesn't really need to be short.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Jason
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe
> > > > > > > > <cm...@apache.org> > >
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > > > > > > > Hi, Colin,
> > > > > > > > > >
> > > > > > > > > > Thanks for the KIP. Looks good to me overall. Just a
> > > > > > > > > > couple of> > more
> > > > > > > > > > comments.
> > > > > > > > > >
> > > > > > > > > > 1. As I mentioned earlier, it might be useful to add
> > > > > > > > > > some> > metrics
> > > > > > for
> > > > > > > > > > monitoring the usage of the session cache. For example,
> > > > > > > > > > it> > would be
> > > > > > > > > useful
> > > > > > > > > > to know how many slots are being used (or unused), # of
> > > > > > > > > > total> > > > > partitions
> > > > > > > > > in
> > > > > > > > > > the cached slots (to understand space), the eviction
> > > > > > > > > > rate (to> > see
> > > > > > if
> > > > > > > > > there
> > > > > > > > > > is any churn), etc.
> > > > > > > > >
> > > > > > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but
> > > > > > > > > I> > forgot
> > > > > > > about
> > > > > > > > > it. I just added some proposed metrics to the KIP wiki.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > 2. Using max_bytes to 0 represent the removal of a
> > > > > > > > > > partition> > seems
> > > > > > > > > > unintuitive. Perhaps it's better to either add a flag
> > > > > > > > > > per> > partition
> > > > > > > or
> > > > > > > > > add
> > > > > > > > > > a removed partition list.
> > > > > > > > >
> > > > > > > > > Perhaps if we use max_bytes -1 to represent removal, it
> > > > > > > > > will be> > more
> > > > > > > > > intuitive? After all, -1 bytes is clearly not a valid
> > > > > > > > > amount of> > > > bytes
> > > > > > > to
> > > > > > > > > fetch. Or should be have a separate array of removed
> > > > > > TopicPartitions?
> > > > > > > > >
> > > > > > > > > On a related note, in the FetchResponse#PartitionData, we
> > > > > > > > > have an> > > > > "error"
> > > > > > > > > field, plus highWatermark, lastStableOffset,
> > > > > > > > > logStartOffset, etc.> > > > But
> > > > > > > when
> > > > > > > > > the "error" field is set, those other fields are not used.>
> > > Perhaps
> > > > > > we
> > > > > > > > > could save some space by just having a separate array of
> > > > "partitions
> > > > > > > with
> > > > > > > > > errors." In the common case where there are no errors,
> > > > > > > > > this> > would
> > > > > > > save 2
> > > > > > > > > bytes per partition, which could be quite significant in
> > > > > > > > > large> > > > > responses.
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Jun
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
> > > > cmccabe@apache.org>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi all,
> > > > > > > > > > >
> > > > > > > > > > > I updated the KIP. There is also an implementation of
> > > > > > > > > > > this> > KIP
> > > > > > > here:
> > > > > > > > > > > https://github.com/apache/kafka/pull/4418
> > > > > > > > > > >
> > > > > > > > > > > The updated implementation simplifies a few things,
> > > > > > > > > > > and adds> > the
> > > > > > > > > ability
> > > > > > > > > > > to incrementally add or remove individual partitions
> > > > > > > > > > > in an> > > > > incremental
> > > > > > > > > > > fetch request.
> > > > > > > > > > >
> > > > > > > > > > > best,
> > > > > > > > > > > Colin
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > > > > > > > > Hi all,
> > > > > > > > > > > >
> > > > > > > > > > > > I'd like to start the vote on KIP-227: Incremental
> > > > > > > > > > > > Fetch> > > > > Requests.
> > > > > > > > > > > >
> > > > > > > > > > > > The KIP is here:
> > > > > > > > > > > > https://cwiki.apache.org/
> confluence/display/KAFKA/KIP->
> > > > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increas-
> > > > > > > > > > > e+> > > > > > > > > Partition+Scalability
> > > > > > > > > > > >
> > > > > > > > > > > > and discussion thread earlier:
> > > > > > > > > > > > https://www.mail-archive.com/
> > dev@kafka.apache.org/msg83115> > .
> > > > > > html
> > > > > > > > > > > >
> > > > > > > > > > > > thanks,
> > > > > > > > > > > > Colin
> > > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > >
> > > >
> >
> >
>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Jun Rao <ju...@confluent.io>.
Since this is a server side metric, it's probably better to use Yammer Rate
(which has count) for consistency.
Thanks,
Jun
On Tue, Jan 23, 2018 at 10:17 PM, Colin McCabe <co...@cmccabe.xyz> wrote:
> On Tue, Jan 23, 2018, at 21:47, Ismael Juma wrote:
> > Colin,
> >
> > You get a cumulative count for rates since we added
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics>
>
> Oh, good point.
>
> C.
>
>
> > Ismael
> >
> > On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe
> > <cm...@apache.org> wrote:>
> > > On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> > > > Hi, Collin,
> > > >
> > > > Thanks for the updated KIP. +1. Just a minor comment. It seems
> > > > that it's> > > better for TotalIncrementalFetchSessionsEvicted to
> be a rate,
> > > > instead of> > > just an ever-growing count.
> > >
> > > Thanks. Perhaps we can add the rate in addition to the total
> > > eviction> > count?
> > >
> > > best,
> > > Colin
> > >
> > > >
> > > > Jun
> > > >
> > > > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson
> > > > <ja...@confluent.io>> > wrote:
> > > >
> > > > > >
> > > > > > What if we want to have fetch sessions for non-incremental
> > > > > > fetches> > in the
> > > > > > future, though? Also, we don't expect this configuration to
> > > > > > be> > changed
> > > > > > often, so it doesn't really need to be short.
> > > > >
> > > > >
> > > > > Hmm.. But in that case, I'm not sure we'd need to distinguish
> > > > > the two> > > > cases. If the non-incremental sessions are
> occupying space
> > > proportional to
> > > > > the fetched partitions, using the same config for both would be> >
> reasonable.
> > > > > If they are not (which is more likely), we probably wouldn't
> > > > > need a> > config
> > > > > at all. Given that, I'd probably still opt for the more concise
> > > > > name.> > It's
> > > > > not a blocker for me though.
> > > > >
> > > > > +1 on the KIP.
> > > > >
> > > > > -Jason
> > > > >
> > > > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe
> > > > > <cm...@apache.org>> > wrote:
> > > > >
> > > > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > > > > > > Hi Colin,
> > > > > > >
> > > > > > > This is looking good to me. A few comments:
> > > > > > >
> > > > > > > 1. The fetch type seems unnecessary in the request and
> > > > > > > response> > schemas
> > > > > > > since it can be inferred by the sessionId/epoch.
> > > > > >
> > > > > > Hi Jason,
> > > > > >
> > > > > > Fair enough... if we need it later, we can always bump the RPC>
> > version.
> > > > > >
> > > > > > > 2. I agree with Jun that a separate array for partitions to
> > > > > > > remove> > > > would
> > > > > > be
> > > > > > > more intuitive.
> > > > > >
> > > > > > OK. I'll switch it to using a separate array.
> > > > > >
> > > > > > > 3. I'm not super thrilled with the cache configuration since
> > > > > > > it> > seems
> > > > > to
> > > > > > > tie us a bit too closely to the implementation. You've
> > > > > > > mostly> > convinced
> > > > > > me
> > > > > > > on the need for the slots config, but I wonder if we can at
> > > > > > > least> > do
> > > > > > > without "min.incremental.fetch.session.eviction.ms"? For
> > > > > > > one, I> > think
> > > > > > the
> > > > > > > broker should reserve the right to evict sessions at will.
> > > > > > > We> > shouldn't
> > > > > > be
> > > > > > > stuck maintaining a small session at the expense of a much
> > > > > > > larger> > one
> > > > > > just
> > > > > > > to enforce this timeout. Internally, I think having some
> > > > > > > cache> > > > stickiness
> > > > > > > to avoid thrashing makes sense, but I think static values
> > > > > > > are> > likely to
> > > > > > be
> > > > > > > good enough and that lets us retain some flexibility to
> > > > > > > change the> > > > > behavior
> > > > > > > in the future.
> > > > > >
> > > > > > OK.
> > > > > >
> > > > > > > 4. I think the word "incremental" is redundant in the config
> > > > > > > names.> > > > Maybe
> > > > > > > it could just be "max.fetch.session.cache.slots" for
> > > > > > > example?> > > > >
> > > > > > What if we want to have fetch sessions for non-incremental
> > > > > > fetches> > in the
> > > > > > future, though? Also, we don't expect this configuration to
> > > > > > be> > changed
> > > > > > often, so it doesn't really need to be short.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jason
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe
> > > > > > > <cm...@apache.org> > >
> > > > > > wrote:
> > > > > > >
> > > > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > > > > > > Hi, Colin,
> > > > > > > > >
> > > > > > > > > Thanks for the KIP. Looks good to me overall. Just a
> > > > > > > > > couple of> > more
> > > > > > > > > comments.
> > > > > > > > >
> > > > > > > > > 1. As I mentioned earlier, it might be useful to add
> > > > > > > > > some> > metrics
> > > > > for
> > > > > > > > > monitoring the usage of the session cache. For example,
> > > > > > > > > it> > would be
> > > > > > > > useful
> > > > > > > > > to know how many slots are being used (or unused), # of
> > > > > > > > > total> > > > > partitions
> > > > > > > > in
> > > > > > > > > the cached slots (to understand space), the eviction
> > > > > > > > > rate (to> > see
> > > > > if
> > > > > > > > there
> > > > > > > > > is any churn), etc.
> > > > > > > >
> > > > > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but
> > > > > > > > I> > forgot
> > > > > > about
> > > > > > > > it. I just added some proposed metrics to the KIP wiki.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > 2. Using max_bytes to 0 represent the removal of a
> > > > > > > > > partition> > seems
> > > > > > > > > unintuitive. Perhaps it's better to either add a flag
> > > > > > > > > per> > partition
> > > > > > or
> > > > > > > > add
> > > > > > > > > a removed partition list.
> > > > > > > >
> > > > > > > > Perhaps if we use max_bytes -1 to represent removal, it
> > > > > > > > will be> > more
> > > > > > > > intuitive? After all, -1 bytes is clearly not a valid
> > > > > > > > amount of> > > > bytes
> > > > > > to
> > > > > > > > fetch. Or should be have a separate array of removed
> > > > > TopicPartitions?
> > > > > > > >
> > > > > > > > On a related note, in the FetchResponse#PartitionData, we
> > > > > > > > have an> > > > > "error"
> > > > > > > > field, plus highWatermark, lastStableOffset,
> > > > > > > > logStartOffset, etc.> > > > But
> > > > > > when
> > > > > > > > the "error" field is set, those other fields are not used.>
> > Perhaps
> > > > > we
> > > > > > > > could save some space by just having a separate array of
> > > "partitions
> > > > > > with
> > > > > > > > errors." In the common case where there are no errors,
> > > > > > > > this> > would
> > > > > > save 2
> > > > > > > > bytes per partition, which could be quite significant in
> > > > > > > > large> > > > > responses.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Jun
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
> > > cmccabe@apache.org>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi all,
> > > > > > > > > >
> > > > > > > > > > I updated the KIP. There is also an implementation of
> > > > > > > > > > this> > KIP
> > > > > > here:
> > > > > > > > > > https://github.com/apache/kafka/pull/4418
> > > > > > > > > >
> > > > > > > > > > The updated implementation simplifies a few things,
> > > > > > > > > > and adds> > the
> > > > > > > > ability
> > > > > > > > > > to incrementally add or remove individual partitions
> > > > > > > > > > in an> > > > > incremental
> > > > > > > > > > fetch request.
> > > > > > > > > >
> > > > > > > > > > best,
> > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > > > > > > > Hi all,
> > > > > > > > > > >
> > > > > > > > > > > I'd like to start the vote on KIP-227: Incremental
> > > > > > > > > > > Fetch> > > > > Requests.
> > > > > > > > > > >
> > > > > > > > > > > The KIP is here:
> > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP->
> > > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increas-
> > > > > > > > > > e+> > > > > > > > > Partition+Scalability
> > > > > > > > > > >
> > > > > > > > > > > and discussion thread earlier:
> > > > > > > > > > > https://www.mail-archive.com/
> dev@kafka.apache.org/msg83115> > .
> > > > > html
> > > > > > > > > > >
> > > > > > > > > > > thanks,
> > > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
>
>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Colin McCabe <cm...@apache.org>.
With 3 +1 votes from Ismael, Jun, and Jason, the vote passes.
Thanks,
Colin
On Tue, Jan 23, 2018, at 22:17, Colin McCabe wrote:
> On Tue, Jan 23, 2018, at 21:47, Ismael Juma wrote:
> > Colin,
> >
> > You get a cumulative count for rates since we added
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics> >
>
> Oh, good point.
>
> C.
>
>
> > Ismael
> >
> > On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe <cm...@apache.org>
> > wrote:> >
> > > On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> > > > Hi, Collin,
> > > >
> > > > Thanks for the updated KIP. +1. Just a minor comment. It seems
> > > > that it's> > > > better for TotalIncrementalFetchSessionsEvicted to be a rate,
> > > > instead of> > > > just an ever-growing count.
> > >
> > > Thanks. Perhaps we can add the rate in addition to the total
> > > eviction> > > count?
> > >
> > > best,
> > > Colin
> > >
> > > >
> > > > Jun
> > > >
> > > > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson
> > > > <ja...@confluent.io>> > > wrote:
> > > >
> > > > > >
> > > > > > What if we want to have fetch sessions for non-incremental
> > > > > > fetches> > > in the
> > > > > > future, though? Also, we don't expect this configuration to
> > > > > > be> > > changed
> > > > > > often, so it doesn't really need to be short.
> > > > >
> > > > >
> > > > > Hmm.. But in that case, I'm not sure we'd need to distinguish
> > > > > the two> > > > > cases. If the non-incremental sessions are occupying space
> > > proportional to
> > > > > the fetched partitions, using the same config for both would
> > > > > be> > > reasonable.
> > > > > If they are not (which is more likely), we probably wouldn't
> > > > > need a> > > config
> > > > > at all. Given that, I'd probably still opt for the more
> > > > > concise name.> > > It's
> > > > > not a blocker for me though.
> > > > >
> > > > > +1 on the KIP.
> > > > >
> > > > > -Jason
> > > > >
> > > > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe
> > > > > <cm...@apache.org>> > > wrote:
> > > > >
> > > > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > > > > > > Hi Colin,
> > > > > > >
> > > > > > > This is looking good to me. A few comments:
> > > > > > >
> > > > > > > 1. The fetch type seems unnecessary in the request and
> > > > > > > response> > > schemas
> > > > > > > since it can be inferred by the sessionId/epoch.
> > > > > >
> > > > > > Hi Jason,
> > > > > >
> > > > > > Fair enough... if we need it later, we can always bump the
> > > > > > RPC> > > version.
> > > > > >
> > > > > > > 2. I agree with Jun that a separate array for partitions
> > > > > > > to remove> > > > > would
> > > > > > be
> > > > > > > more intuitive.
> > > > > >
> > > > > > OK. I'll switch it to using a separate array.
> > > > > >
> > > > > > > 3. I'm not super thrilled with the cache configuration
> > > > > > > since it> > > seems
> > > > > to
> > > > > > > tie us a bit too closely to the implementation. You've
> > > > > > > mostly> > > convinced
> > > > > > me
> > > > > > > on the need for the slots config, but I wonder if we can
> > > > > > > at least> > > do
> > > > > > > without "min.incremental.fetch.session.eviction.ms"? For
> > > > > > > one, I> > > think
> > > > > > the
> > > > > > > broker should reserve the right to evict sessions at will.
> > > > > > > We> > > shouldn't
> > > > > > be
> > > > > > > stuck maintaining a small session at the expense of a much
> > > > > > > larger> > > one
> > > > > > just
> > > > > > > to enforce this timeout. Internally, I think having some
> > > > > > > cache> > > > > stickiness
> > > > > > > to avoid thrashing makes sense, but I think static values
> > > > > > > are> > > likely to
> > > > > > be
> > > > > > > good enough and that lets us retain some flexibility to
> > > > > > > change the> > > > > > behavior
> > > > > > > in the future.
> > > > > >
> > > > > > OK.
> > > > > >
> > > > > > > 4. I think the word "incremental" is redundant in the
> > > > > > > config names.> > > > > Maybe
> > > > > > > it could just be "max.fetch.session.cache.slots" for
> > > > > > > example?> > > > > >
> > > > > > What if we want to have fetch sessions for non-incremental
> > > > > > fetches> > > in the
> > > > > > future, though? Also, we don't expect this configuration to
> > > > > > be> > > changed
> > > > > > often, so it doesn't really need to be short.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Jason
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe
> > > > > > > <cm...@apache.org> > > >
> > > > > > wrote:
> > > > > > >
> > > > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > > > > > > Hi, Colin,
> > > > > > > > >
> > > > > > > > > Thanks for the KIP. Looks good to me overall. Just a
> > > > > > > > > couple of> > > more
> > > > > > > > > comments.
> > > > > > > > >
> > > > > > > > > 1. As I mentioned earlier, it might be useful to add
> > > > > > > > > some> > > metrics
> > > > > for
> > > > > > > > > monitoring the usage of the session cache. For
> > > > > > > > > example, it> > > would be
> > > > > > > > useful
> > > > > > > > > to know how many slots are being used (or unused), #
> > > > > > > > > of total> > > > > > partitions
> > > > > > > > in
> > > > > > > > > the cached slots (to understand space), the eviction
> > > > > > > > > rate (to> > > see
> > > > > if
> > > > > > > > there
> > > > > > > > > is any churn), etc.
> > > > > > > >
> > > > > > > > Thanks, Jun. Sorry-- I meant to address this earlier,
> > > > > > > > but I> > > forgot
> > > > > > about
> > > > > > > > it. I just added some proposed metrics to the KIP wiki.> > > > > > > >
> > > > > > > > >
> > > > > > > > > 2. Using max_bytes to 0 represent the removal of a
> > > > > > > > > partition> > > seems
> > > > > > > > > unintuitive. Perhaps it's better to either add a flag
> > > > > > > > > per> > > partition
> > > > > > or
> > > > > > > > add
> > > > > > > > > a removed partition list.
> > > > > > > >
> > > > > > > > Perhaps if we use max_bytes -1 to represent removal, it
> > > > > > > > will be> > > more
> > > > > > > > intuitive? After all, -1 bytes is clearly not a valid
> > > > > > > > amount of> > > > > bytes
> > > > > > to
> > > > > > > > fetch. Or should be have a separate array of removed
> > > > > TopicPartitions?
> > > > > > > >
> > > > > > > > On a related note, in the FetchResponse#PartitionData,
> > > > > > > > we have an> > > > > > "error"
> > > > > > > > field, plus highWatermark, lastStableOffset,
> > > > > > > > logStartOffset, etc.> > > > > But
> > > > > > when
> > > > > > > > the "error" field is set, those other fields are not
> > > > > > > > used.> > > Perhaps
> > > > > we
> > > > > > > > could save some space by just having a separate array of> > > "partitions
> > > > > > with
> > > > > > > > errors." In the common case where there are no errors,
> > > > > > > > this> > > would
> > > > > > save 2
> > > > > > > > bytes per partition, which could be quite significant in
> > > > > > > > large> > > > > > responses.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Jun
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
> > > cmccabe@apache.org>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi all,
> > > > > > > > > >
> > > > > > > > > > I updated the KIP. There is also an implementation
> > > > > > > > > > of this> > > KIP
> > > > > > here:
> > > > > > > > > > https://github.com/apache/kafka/pull/4418
> > > > > > > > > >
> > > > > > > > > > The updated implementation simplifies a few things,
> > > > > > > > > > and adds> > > the
> > > > > > > > ability
> > > > > > > > > > to incrementally add or remove individual partitions
> > > > > > > > > > in an> > > > > > incremental
> > > > > > > > > > fetch request.
> > > > > > > > > >
> > > > > > > > > > best,
> > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:> > > > > > > > > > > Hi all,
> > > > > > > > > > >
> > > > > > > > > > > I'd like to start the vote on KIP-227: Incremental
> > > > > > > > > > > Fetch> > > > > > Requests.
> > > > > > > > > > >
> > > > > > > > > > > The KIP is here:
> > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-> > > > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Incre-
> > > > > > > > > > ase+> > > > > > > > > > Partition+Scalability
> > > > > > > > > > >
> > > > > > > > > > > and discussion thread earlier:
> > > > > > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115> > > .
> > > > > html
> > > > > > > > > > >
> > > > > > > > > > > thanks,
> > > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Colin McCabe <co...@cmccabe.xyz>.
On Tue, Jan 23, 2018, at 21:47, Ismael Juma wrote:
> Colin,
>
> You get a cumulative count for rates since we added
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics>
Oh, good point.
C.
> Ismael
>
> On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe
> <cm...@apache.org> wrote:>
> > On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> > > Hi, Collin,
> > >
> > > Thanks for the updated KIP. +1. Just a minor comment. It seems
> > > that it's> > > better for TotalIncrementalFetchSessionsEvicted to be a rate,
> > > instead of> > > just an ever-growing count.
> >
> > Thanks. Perhaps we can add the rate in addition to the total
> > eviction> > count?
> >
> > best,
> > Colin
> >
> > >
> > > Jun
> > >
> > > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson
> > > <ja...@confluent.io>> > wrote:
> > >
> > > > >
> > > > > What if we want to have fetch sessions for non-incremental
> > > > > fetches> > in the
> > > > > future, though? Also, we don't expect this configuration to
> > > > > be> > changed
> > > > > often, so it doesn't really need to be short.
> > > >
> > > >
> > > > Hmm.. But in that case, I'm not sure we'd need to distinguish
> > > > the two> > > > cases. If the non-incremental sessions are occupying space
> > proportional to
> > > > the fetched partitions, using the same config for both would be> > reasonable.
> > > > If they are not (which is more likely), we probably wouldn't
> > > > need a> > config
> > > > at all. Given that, I'd probably still opt for the more concise
> > > > name.> > It's
> > > > not a blocker for me though.
> > > >
> > > > +1 on the KIP.
> > > >
> > > > -Jason
> > > >
> > > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe
> > > > <cm...@apache.org>> > wrote:
> > > >
> > > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > > > > > Hi Colin,
> > > > > >
> > > > > > This is looking good to me. A few comments:
> > > > > >
> > > > > > 1. The fetch type seems unnecessary in the request and
> > > > > > response> > schemas
> > > > > > since it can be inferred by the sessionId/epoch.
> > > > >
> > > > > Hi Jason,
> > > > >
> > > > > Fair enough... if we need it later, we can always bump the RPC> > version.
> > > > >
> > > > > > 2. I agree with Jun that a separate array for partitions to
> > > > > > remove> > > > would
> > > > > be
> > > > > > more intuitive.
> > > > >
> > > > > OK. I'll switch it to using a separate array.
> > > > >
> > > > > > 3. I'm not super thrilled with the cache configuration since
> > > > > > it> > seems
> > > > to
> > > > > > tie us a bit too closely to the implementation. You've
> > > > > > mostly> > convinced
> > > > > me
> > > > > > on the need for the slots config, but I wonder if we can at
> > > > > > least> > do
> > > > > > without "min.incremental.fetch.session.eviction.ms"? For
> > > > > > one, I> > think
> > > > > the
> > > > > > broker should reserve the right to evict sessions at will.
> > > > > > We> > shouldn't
> > > > > be
> > > > > > stuck maintaining a small session at the expense of a much
> > > > > > larger> > one
> > > > > just
> > > > > > to enforce this timeout. Internally, I think having some
> > > > > > cache> > > > stickiness
> > > > > > to avoid thrashing makes sense, but I think static values
> > > > > > are> > likely to
> > > > > be
> > > > > > good enough and that lets us retain some flexibility to
> > > > > > change the> > > > > behavior
> > > > > > in the future.
> > > > >
> > > > > OK.
> > > > >
> > > > > > 4. I think the word "incremental" is redundant in the config
> > > > > > names.> > > > Maybe
> > > > > > it could just be "max.fetch.session.cache.slots" for
> > > > > > example?> > > > >
> > > > > What if we want to have fetch sessions for non-incremental
> > > > > fetches> > in the
> > > > > future, though? Also, we don't expect this configuration to
> > > > > be> > changed
> > > > > often, so it doesn't really need to be short.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Jason
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe
> > > > > > <cm...@apache.org> > >
> > > > > wrote:
> > > > > >
> > > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > > > > > Hi, Colin,
> > > > > > > >
> > > > > > > > Thanks for the KIP. Looks good to me overall. Just a
> > > > > > > > couple of> > more
> > > > > > > > comments.
> > > > > > > >
> > > > > > > > 1. As I mentioned earlier, it might be useful to add
> > > > > > > > some> > metrics
> > > > for
> > > > > > > > monitoring the usage of the session cache. For example,
> > > > > > > > it> > would be
> > > > > > > useful
> > > > > > > > to know how many slots are being used (or unused), # of
> > > > > > > > total> > > > > partitions
> > > > > > > in
> > > > > > > > the cached slots (to understand space), the eviction
> > > > > > > > rate (to> > see
> > > > if
> > > > > > > there
> > > > > > > > is any churn), etc.
> > > > > > >
> > > > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but
> > > > > > > I> > forgot
> > > > > about
> > > > > > > it. I just added some proposed metrics to the KIP wiki.
> > > > > > >
> > > > > > > >
> > > > > > > > 2. Using max_bytes to 0 represent the removal of a
> > > > > > > > partition> > seems
> > > > > > > > unintuitive. Perhaps it's better to either add a flag
> > > > > > > > per> > partition
> > > > > or
> > > > > > > add
> > > > > > > > a removed partition list.
> > > > > > >
> > > > > > > Perhaps if we use max_bytes -1 to represent removal, it
> > > > > > > will be> > more
> > > > > > > intuitive? After all, -1 bytes is clearly not a valid
> > > > > > > amount of> > > > bytes
> > > > > to
> > > > > > > fetch. Or should be have a separate array of removed
> > > > TopicPartitions?
> > > > > > >
> > > > > > > On a related note, in the FetchResponse#PartitionData, we
> > > > > > > have an> > > > > "error"
> > > > > > > field, plus highWatermark, lastStableOffset,
> > > > > > > logStartOffset, etc.> > > > But
> > > > > when
> > > > > > > the "error" field is set, those other fields are not used.> > Perhaps
> > > > we
> > > > > > > could save some space by just having a separate array of
> > "partitions
> > > > > with
> > > > > > > errors." In the common case where there are no errors,
> > > > > > > this> > would
> > > > > save 2
> > > > > > > bytes per partition, which could be quite significant in
> > > > > > > large> > > > > responses.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > Jun
> > > > > > > >
> > > > > > > >
> > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
> > cmccabe@apache.org>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi all,
> > > > > > > > >
> > > > > > > > > I updated the KIP. There is also an implementation of
> > > > > > > > > this> > KIP
> > > > > here:
> > > > > > > > > https://github.com/apache/kafka/pull/4418
> > > > > > > > >
> > > > > > > > > The updated implementation simplifies a few things,
> > > > > > > > > and adds> > the
> > > > > > > ability
> > > > > > > > > to incrementally add or remove individual partitions
> > > > > > > > > in an> > > > > incremental
> > > > > > > > > fetch request.
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > > > > > > Hi all,
> > > > > > > > > >
> > > > > > > > > > I'd like to start the vote on KIP-227: Incremental
> > > > > > > > > > Fetch> > > > > Requests.
> > > > > > > > > >
> > > > > > > > > > The KIP is here:
> > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-> > > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increas-
> > > > > > > > > e+> > > > > > > > > Partition+Scalability
> > > > > > > > > >
> > > > > > > > > > and discussion thread earlier:
> > > > > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115> > .
> > > > html
> > > > > > > > > >
> > > > > > > > > > thanks,
> > > > > > > > > > Colin
> > > > > > > > >
> > > > > > >
> > > > >
> > > >
> >
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Ismael Juma <is...@juma.me.uk>.
Colin,
You get a cumulative count for rates since we added
https://cwiki.apache.org/confluence/display/KAFKA/KIP-187+-+Add+cumulative+count+metric+for+all+Kafka+rate+metrics
Ismael
On Tue, Jan 23, 2018 at 4:21 PM, Colin McCabe <cm...@apache.org> wrote:
> On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> > Hi, Collin,
> >
> > Thanks for the updated KIP. +1. Just a minor comment. It seems that it's
> > better for TotalIncrementalFetchSessionsEvicted to be a rate, instead of
> > just an ever-growing count.
>
> Thanks. Perhaps we can add the rate in addition to the total eviction
> count?
>
> best,
> Colin
>
> >
> > Jun
> >
> > On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson <ja...@confluent.io>
> wrote:
> >
> > > >
> > > > What if we want to have fetch sessions for non-incremental fetches
> in the
> > > > future, though? Also, we don't expect this configuration to be
> changed
> > > > often, so it doesn't really need to be short.
> > >
> > >
> > > Hmm.. But in that case, I'm not sure we'd need to distinguish the two
> > > cases. If the non-incremental sessions are occupying space
> proportional to
> > > the fetched partitions, using the same config for both would be
> reasonable.
> > > If they are not (which is more likely), we probably wouldn't need a
> config
> > > at all. Given that, I'd probably still opt for the more concise name.
> It's
> > > not a blocker for me though.
> > >
> > > +1 on the KIP.
> > >
> > > -Jason
> > >
> > > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe <cm...@apache.org>
> wrote:
> > >
> > > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > > > > Hi Colin,
> > > > >
> > > > > This is looking good to me. A few comments:
> > > > >
> > > > > 1. The fetch type seems unnecessary in the request and response
> schemas
> > > > > since it can be inferred by the sessionId/epoch.
> > > >
> > > > Hi Jason,
> > > >
> > > > Fair enough... if we need it later, we can always bump the RPC
> version.
> > > >
> > > > > 2. I agree with Jun that a separate array for partitions to remove
> > > would
> > > > be
> > > > > more intuitive.
> > > >
> > > > OK. I'll switch it to using a separate array.
> > > >
> > > > > 3. I'm not super thrilled with the cache configuration since it
> seems
> > > to
> > > > > tie us a bit too closely to the implementation. You've mostly
> convinced
> > > > me
> > > > > on the need for the slots config, but I wonder if we can at least
> do
> > > > > without "min.incremental.fetch.session.eviction.ms"? For one, I
> think
> > > > the
> > > > > broker should reserve the right to evict sessions at will. We
> shouldn't
> > > > be
> > > > > stuck maintaining a small session at the expense of a much larger
> one
> > > > just
> > > > > to enforce this timeout. Internally, I think having some cache
> > > stickiness
> > > > > to avoid thrashing makes sense, but I think static values are
> likely to
> > > > be
> > > > > good enough and that lets us retain some flexibility to change the
> > > > behavior
> > > > > in the future.
> > > >
> > > > OK.
> > > >
> > > > > 4. I think the word "incremental" is redundant in the config names.
> > > Maybe
> > > > > it could just be "max.fetch.session.cache.slots" for example?
> > > >
> > > > What if we want to have fetch sessions for non-incremental fetches
> in the
> > > > future, though? Also, we don't expect this configuration to be
> changed
> > > > often, so it doesn't really need to be short.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > > Thanks,
> > > > > Jason
> > > > >
> > > > >
> > > > >
> > > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe <cmccabe@apache.org
> >
> > > > wrote:
> > > > >
> > > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > > > > Hi, Colin,
> > > > > > >
> > > > > > > Thanks for the KIP. Looks good to me overall. Just a couple of
> more
> > > > > > > comments.
> > > > > > >
> > > > > > > 1. As I mentioned earlier, it might be useful to add some
> metrics
> > > for
> > > > > > > monitoring the usage of the session cache. For example, it
> would be
> > > > > > useful
> > > > > > > to know how many slots are being used (or unused), # of total
> > > > partitions
> > > > > > in
> > > > > > > the cached slots (to understand space), the eviction rate (to
> see
> > > if
> > > > > > there
> > > > > > > is any churn), etc.
> > > > > >
> > > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but I
> forgot
> > > > about
> > > > > > it. I just added some proposed metrics to the KIP wiki.
> > > > > >
> > > > > > >
> > > > > > > 2. Using max_bytes to 0 represent the removal of a partition
> seems
> > > > > > > unintuitive. Perhaps it's better to either add a flag per
> partition
> > > > or
> > > > > > add
> > > > > > > a removed partition list.
> > > > > >
> > > > > > Perhaps if we use max_bytes -1 to represent removal, it will be
> more
> > > > > > intuitive? After all, -1 bytes is clearly not a valid amount of
> > > bytes
> > > > to
> > > > > > fetch. Or should be have a separate array of removed
> > > TopicPartitions?
> > > > > >
> > > > > > On a related note, in the FetchResponse#PartitionData, we have an
> > > > "error"
> > > > > > field, plus highWatermark, lastStableOffset, logStartOffset, etc.
> > > But
> > > > when
> > > > > > the "error" field is set, those other fields are not used.
> Perhaps
> > > we
> > > > > > could save some space by just having a separate array of
> "partitions
> > > > with
> > > > > > errors." In the common case where there are no errors, this
> would
> > > > save 2
> > > > > > bytes per partition, which could be quite significant in large
> > > > responses.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > Jun
> > > > > > >
> > > > > > >
> > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <
> cmccabe@apache.org>
> > > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I updated the KIP. There is also an implementation of this
> KIP
> > > > here:
> > > > > > > > https://github.com/apache/kafka/pull/4418
> > > > > > > >
> > > > > > > > The updated implementation simplifies a few things, and adds
> the
> > > > > > ability
> > > > > > > > to incrementally add or remove individual partitions in an
> > > > incremental
> > > > > > > > fetch request.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > >
> > > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > > > > > Hi all,
> > > > > > > > >
> > > > > > > > > I'd like to start the vote on KIP-227: Incremental Fetch
> > > > Requests.
> > > > > > > > >
> > > > > > > > > The KIP is here:
> > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> > > > > > > > Partition+Scalability
> > > > > > > > >
> > > > > > > > > and discussion thread earlier:
> > > > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115
> .
> > > html
> > > > > > > > >
> > > > > > > > > thanks,
> > > > > > > > > Colin
> > > > > > > >
> > > > > >
> > > >
> > >
>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Colin McCabe <cm...@apache.org>.
On Tue, Jan 23, 2018, at 11:57, Jun Rao wrote:
> Hi, Collin,
>
> Thanks for the updated KIP. +1. Just a minor comment. It seems that it's
> better for TotalIncrementalFetchSessionsEvicted to be a rate, instead of
> just an ever-growing count.
Thanks. Perhaps we can add the rate in addition to the total eviction count?
best,
Colin
>
> Jun
>
> On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson <ja...@confluent.io> wrote:
>
> > >
> > > What if we want to have fetch sessions for non-incremental fetches in the
> > > future, though? Also, we don't expect this configuration to be changed
> > > often, so it doesn't really need to be short.
> >
> >
> > Hmm.. But in that case, I'm not sure we'd need to distinguish the two
> > cases. If the non-incremental sessions are occupying space proportional to
> > the fetched partitions, using the same config for both would be reasonable.
> > If they are not (which is more likely), we probably wouldn't need a config
> > at all. Given that, I'd probably still opt for the more concise name. It's
> > not a blocker for me though.
> >
> > +1 on the KIP.
> >
> > -Jason
> >
> > On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe <cm...@apache.org> wrote:
> >
> > > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > > > Hi Colin,
> > > >
> > > > This is looking good to me. A few comments:
> > > >
> > > > 1. The fetch type seems unnecessary in the request and response schemas
> > > > since it can be inferred by the sessionId/epoch.
> > >
> > > Hi Jason,
> > >
> > > Fair enough... if we need it later, we can always bump the RPC version.
> > >
> > > > 2. I agree with Jun that a separate array for partitions to remove
> > would
> > > be
> > > > more intuitive.
> > >
> > > OK. I'll switch it to using a separate array.
> > >
> > > > 3. I'm not super thrilled with the cache configuration since it seems
> > to
> > > > tie us a bit too closely to the implementation. You've mostly convinced
> > > me
> > > > on the need for the slots config, but I wonder if we can at least do
> > > > without "min.incremental.fetch.session.eviction.ms"? For one, I think
> > > the
> > > > broker should reserve the right to evict sessions at will. We shouldn't
> > > be
> > > > stuck maintaining a small session at the expense of a much larger one
> > > just
> > > > to enforce this timeout. Internally, I think having some cache
> > stickiness
> > > > to avoid thrashing makes sense, but I think static values are likely to
> > > be
> > > > good enough and that lets us retain some flexibility to change the
> > > behavior
> > > > in the future.
> > >
> > > OK.
> > >
> > > > 4. I think the word "incremental" is redundant in the config names.
> > Maybe
> > > > it could just be "max.fetch.session.cache.slots" for example?
> > >
> > > What if we want to have fetch sessions for non-incremental fetches in the
> > > future, though? Also, we don't expect this configuration to be changed
> > > often, so it doesn't really need to be short.
> > >
> > > best,
> > > Colin
> > >
> > > >
> > > > Thanks,
> > > > Jason
> > > >
> > > >
> > > >
> > > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe <cm...@apache.org>
> > > wrote:
> > > >
> > > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > > > Hi, Colin,
> > > > > >
> > > > > > Thanks for the KIP. Looks good to me overall. Just a couple of more
> > > > > > comments.
> > > > > >
> > > > > > 1. As I mentioned earlier, it might be useful to add some metrics
> > for
> > > > > > monitoring the usage of the session cache. For example, it would be
> > > > > useful
> > > > > > to know how many slots are being used (or unused), # of total
> > > partitions
> > > > > in
> > > > > > the cached slots (to understand space), the eviction rate (to see
> > if
> > > > > there
> > > > > > is any churn), etc.
> > > > >
> > > > > Thanks, Jun. Sorry-- I meant to address this earlier, but I forgot
> > > about
> > > > > it. I just added some proposed metrics to the KIP wiki.
> > > > >
> > > > > >
> > > > > > 2. Using max_bytes to 0 represent the removal of a partition seems
> > > > > > unintuitive. Perhaps it's better to either add a flag per partition
> > > or
> > > > > add
> > > > > > a removed partition list.
> > > > >
> > > > > Perhaps if we use max_bytes -1 to represent removal, it will be more
> > > > > intuitive? After all, -1 bytes is clearly not a valid amount of
> > bytes
> > > to
> > > > > fetch. Or should be have a separate array of removed
> > TopicPartitions?
> > > > >
> > > > > On a related note, in the FetchResponse#PartitionData, we have an
> > > "error"
> > > > > field, plus highWatermark, lastStableOffset, logStartOffset, etc.
> > But
> > > when
> > > > > the "error" field is set, those other fields are not used. Perhaps
> > we
> > > > > could save some space by just having a separate array of "partitions
> > > with
> > > > > errors." In the common case where there are no errors, this would
> > > save 2
> > > > > bytes per partition, which could be quite significant in large
> > > responses.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > > >
> > > > > > Jun
> > > > > >
> > > > > >
> > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <cm...@apache.org>
> > > > > wrote:
> > > > > >
> > > > > > > Hi all,
> > > > > > >
> > > > > > > I updated the KIP. There is also an implementation of this KIP
> > > here:
> > > > > > > https://github.com/apache/kafka/pull/4418
> > > > > > >
> > > > > > > The updated implementation simplifies a few things, and adds the
> > > > > ability
> > > > > > > to incrementally add or remove individual partitions in an
> > > incremental
> > > > > > > fetch request.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > >
> > > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I'd like to start the vote on KIP-227: Incremental Fetch
> > > Requests.
> > > > > > > >
> > > > > > > > The KIP is here:
> > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> > > > > > > Partition+Scalability
> > > > > > > >
> > > > > > > > and discussion thread earlier:
> > > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.
> > html
> > > > > > > >
> > > > > > > > thanks,
> > > > > > > > Colin
> > > > > > >
> > > > >
> > >
> >
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Jun Rao <ju...@confluent.io>.
Hi, Collin,
Thanks for the updated KIP. +1. Just a minor comment. It seems that it's
better for TotalIncrementalFetchSessionsEvicted to be a rate, instead of
just an ever-growing count.
Jun
On Mon, Jan 22, 2018 at 4:35 PM, Jason Gustafson <ja...@confluent.io> wrote:
> >
> > What if we want to have fetch sessions for non-incremental fetches in the
> > future, though? Also, we don't expect this configuration to be changed
> > often, so it doesn't really need to be short.
>
>
> Hmm.. But in that case, I'm not sure we'd need to distinguish the two
> cases. If the non-incremental sessions are occupying space proportional to
> the fetched partitions, using the same config for both would be reasonable.
> If they are not (which is more likely), we probably wouldn't need a config
> at all. Given that, I'd probably still opt for the more concise name. It's
> not a blocker for me though.
>
> +1 on the KIP.
>
> -Jason
>
> On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe <cm...@apache.org> wrote:
>
> > On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > > Hi Colin,
> > >
> > > This is looking good to me. A few comments:
> > >
> > > 1. The fetch type seems unnecessary in the request and response schemas
> > > since it can be inferred by the sessionId/epoch.
> >
> > Hi Jason,
> >
> > Fair enough... if we need it later, we can always bump the RPC version.
> >
> > > 2. I agree with Jun that a separate array for partitions to remove
> would
> > be
> > > more intuitive.
> >
> > OK. I'll switch it to using a separate array.
> >
> > > 3. I'm not super thrilled with the cache configuration since it seems
> to
> > > tie us a bit too closely to the implementation. You've mostly convinced
> > me
> > > on the need for the slots config, but I wonder if we can at least do
> > > without "min.incremental.fetch.session.eviction.ms"? For one, I think
> > the
> > > broker should reserve the right to evict sessions at will. We shouldn't
> > be
> > > stuck maintaining a small session at the expense of a much larger one
> > just
> > > to enforce this timeout. Internally, I think having some cache
> stickiness
> > > to avoid thrashing makes sense, but I think static values are likely to
> > be
> > > good enough and that lets us retain some flexibility to change the
> > behavior
> > > in the future.
> >
> > OK.
> >
> > > 4. I think the word "incremental" is redundant in the config names.
> Maybe
> > > it could just be "max.fetch.session.cache.slots" for example?
> >
> > What if we want to have fetch sessions for non-incremental fetches in the
> > future, though? Also, we don't expect this configuration to be changed
> > often, so it doesn't really need to be short.
> >
> > best,
> > Colin
> >
> > >
> > > Thanks,
> > > Jason
> > >
> > >
> > >
> > > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe <cm...@apache.org>
> > wrote:
> > >
> > > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > > Hi, Colin,
> > > > >
> > > > > Thanks for the KIP. Looks good to me overall. Just a couple of more
> > > > > comments.
> > > > >
> > > > > 1. As I mentioned earlier, it might be useful to add some metrics
> for
> > > > > monitoring the usage of the session cache. For example, it would be
> > > > useful
> > > > > to know how many slots are being used (or unused), # of total
> > partitions
> > > > in
> > > > > the cached slots (to understand space), the eviction rate (to see
> if
> > > > there
> > > > > is any churn), etc.
> > > >
> > > > Thanks, Jun. Sorry-- I meant to address this earlier, but I forgot
> > about
> > > > it. I just added some proposed metrics to the KIP wiki.
> > > >
> > > > >
> > > > > 2. Using max_bytes to 0 represent the removal of a partition seems
> > > > > unintuitive. Perhaps it's better to either add a flag per partition
> > or
> > > > add
> > > > > a removed partition list.
> > > >
> > > > Perhaps if we use max_bytes -1 to represent removal, it will be more
> > > > intuitive? After all, -1 bytes is clearly not a valid amount of
> bytes
> > to
> > > > fetch. Or should be have a separate array of removed
> TopicPartitions?
> > > >
> > > > On a related note, in the FetchResponse#PartitionData, we have an
> > "error"
> > > > field, plus highWatermark, lastStableOffset, logStartOffset, etc.
> But
> > when
> > > > the "error" field is set, those other fields are not used. Perhaps
> we
> > > > could save some space by just having a separate array of "partitions
> > with
> > > > errors." In the common case where there are no errors, this would
> > save 2
> > > > bytes per partition, which could be quite significant in large
> > responses.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > > Jun
> > > > >
> > > > >
> > > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <cm...@apache.org>
> > > > wrote:
> > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > I updated the KIP. There is also an implementation of this KIP
> > here:
> > > > > > https://github.com/apache/kafka/pull/4418
> > > > > >
> > > > > > The updated implementation simplifies a few things, and adds the
> > > > ability
> > > > > > to incrementally add or remove individual partitions in an
> > incremental
> > > > > > fetch request.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > >
> > > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > > > Hi all,
> > > > > > >
> > > > > > > I'd like to start the vote on KIP-227: Incremental Fetch
> > Requests.
> > > > > > >
> > > > > > > The KIP is here:
> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> > > > > > Partition+Scalability
> > > > > > >
> > > > > > > and discussion thread earlier:
> > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.
> html
> > > > > > >
> > > > > > > thanks,
> > > > > > > Colin
> > > > > >
> > > >
> >
>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Jason Gustafson <ja...@confluent.io>.
>
> What if we want to have fetch sessions for non-incremental fetches in the
> future, though? Also, we don't expect this configuration to be changed
> often, so it doesn't really need to be short.
Hmm.. But in that case, I'm not sure we'd need to distinguish the two
cases. If the non-incremental sessions are occupying space proportional to
the fetched partitions, using the same config for both would be reasonable.
If they are not (which is more likely), we probably wouldn't need a config
at all. Given that, I'd probably still opt for the more concise name. It's
not a blocker for me though.
+1 on the KIP.
-Jason
On Mon, Jan 22, 2018 at 3:56 PM, Colin McCabe <cm...@apache.org> wrote:
> On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> > Hi Colin,
> >
> > This is looking good to me. A few comments:
> >
> > 1. The fetch type seems unnecessary in the request and response schemas
> > since it can be inferred by the sessionId/epoch.
>
> Hi Jason,
>
> Fair enough... if we need it later, we can always bump the RPC version.
>
> > 2. I agree with Jun that a separate array for partitions to remove would
> be
> > more intuitive.
>
> OK. I'll switch it to using a separate array.
>
> > 3. I'm not super thrilled with the cache configuration since it seems to
> > tie us a bit too closely to the implementation. You've mostly convinced
> me
> > on the need for the slots config, but I wonder if we can at least do
> > without "min.incremental.fetch.session.eviction.ms"? For one, I think
> the
> > broker should reserve the right to evict sessions at will. We shouldn't
> be
> > stuck maintaining a small session at the expense of a much larger one
> just
> > to enforce this timeout. Internally, I think having some cache stickiness
> > to avoid thrashing makes sense, but I think static values are likely to
> be
> > good enough and that lets us retain some flexibility to change the
> behavior
> > in the future.
>
> OK.
>
> > 4. I think the word "incremental" is redundant in the config names. Maybe
> > it could just be "max.fetch.session.cache.slots" for example?
>
> What if we want to have fetch sessions for non-incremental fetches in the
> future, though? Also, we don't expect this configuration to be changed
> often, so it doesn't really need to be short.
>
> best,
> Colin
>
> >
> > Thanks,
> > Jason
> >
> >
> >
> > On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe <cm...@apache.org>
> wrote:
> >
> > > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > > Hi, Colin,
> > > >
> > > > Thanks for the KIP. Looks good to me overall. Just a couple of more
> > > > comments.
> > > >
> > > > 1. As I mentioned earlier, it might be useful to add some metrics for
> > > > monitoring the usage of the session cache. For example, it would be
> > > useful
> > > > to know how many slots are being used (or unused), # of total
> partitions
> > > in
> > > > the cached slots (to understand space), the eviction rate (to see if
> > > there
> > > > is any churn), etc.
> > >
> > > Thanks, Jun. Sorry-- I meant to address this earlier, but I forgot
> about
> > > it. I just added some proposed metrics to the KIP wiki.
> > >
> > > >
> > > > 2. Using max_bytes to 0 represent the removal of a partition seems
> > > > unintuitive. Perhaps it's better to either add a flag per partition
> or
> > > add
> > > > a removed partition list.
> > >
> > > Perhaps if we use max_bytes -1 to represent removal, it will be more
> > > intuitive? After all, -1 bytes is clearly not a valid amount of bytes
> to
> > > fetch. Or should be have a separate array of removed TopicPartitions?
> > >
> > > On a related note, in the FetchResponse#PartitionData, we have an
> "error"
> > > field, plus highWatermark, lastStableOffset, logStartOffset, etc. But
> when
> > > the "error" field is set, those other fields are not used. Perhaps we
> > > could save some space by just having a separate array of "partitions
> with
> > > errors." In the common case where there are no errors, this would
> save 2
> > > bytes per partition, which could be quite significant in large
> responses.
> > >
> > > best,
> > > Colin
> > >
> > > >
> > > > Jun
> > > >
> > > >
> > > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <cm...@apache.org>
> > > wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > I updated the KIP. There is also an implementation of this KIP
> here:
> > > > > https://github.com/apache/kafka/pull/4418
> > > > >
> > > > > The updated implementation simplifies a few things, and adds the
> > > ability
> > > > > to incrementally add or remove individual partitions in an
> incremental
> > > > > fetch request.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > I'd like to start the vote on KIP-227: Incremental Fetch
> Requests.
> > > > > >
> > > > > > The KIP is here:
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> > > > > Partition+Scalability
> > > > > >
> > > > > > and discussion thread earlier:
> > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.html
> > > > > >
> > > > > > thanks,
> > > > > > Colin
> > > > >
> > >
>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Colin McCabe <cm...@apache.org>.
On Mon, Jan 22, 2018, at 15:42, Jason Gustafson wrote:
> Hi Colin,
>
> This is looking good to me. A few comments:
>
> 1. The fetch type seems unnecessary in the request and response schemas
> since it can be inferred by the sessionId/epoch.
Hi Jason,
Fair enough... if we need it later, we can always bump the RPC version.
> 2. I agree with Jun that a separate array for partitions to remove would be
> more intuitive.
OK. I'll switch it to using a separate array.
> 3. I'm not super thrilled with the cache configuration since it seems to
> tie us a bit too closely to the implementation. You've mostly convinced me
> on the need for the slots config, but I wonder if we can at least do
> without "min.incremental.fetch.session.eviction.ms"? For one, I think the
> broker should reserve the right to evict sessions at will. We shouldn't be
> stuck maintaining a small session at the expense of a much larger one just
> to enforce this timeout. Internally, I think having some cache stickiness
> to avoid thrashing makes sense, but I think static values are likely to be
> good enough and that lets us retain some flexibility to change the behavior
> in the future.
OK.
> 4. I think the word "incremental" is redundant in the config names. Maybe
> it could just be "max.fetch.session.cache.slots" for example?
What if we want to have fetch sessions for non-incremental fetches in the future, though? Also, we don't expect this configuration to be changed often, so it doesn't really need to be short.
best,
Colin
>
> Thanks,
> Jason
>
>
>
> On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe <cm...@apache.org> wrote:
>
> > On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > > Hi, Colin,
> > >
> > > Thanks for the KIP. Looks good to me overall. Just a couple of more
> > > comments.
> > >
> > > 1. As I mentioned earlier, it might be useful to add some metrics for
> > > monitoring the usage of the session cache. For example, it would be
> > useful
> > > to know how many slots are being used (or unused), # of total partitions
> > in
> > > the cached slots (to understand space), the eviction rate (to see if
> > there
> > > is any churn), etc.
> >
> > Thanks, Jun. Sorry-- I meant to address this earlier, but I forgot about
> > it. I just added some proposed metrics to the KIP wiki.
> >
> > >
> > > 2. Using max_bytes to 0 represent the removal of a partition seems
> > > unintuitive. Perhaps it's better to either add a flag per partition or
> > add
> > > a removed partition list.
> >
> > Perhaps if we use max_bytes -1 to represent removal, it will be more
> > intuitive? After all, -1 bytes is clearly not a valid amount of bytes to
> > fetch. Or should be have a separate array of removed TopicPartitions?
> >
> > On a related note, in the FetchResponse#PartitionData, we have an "error"
> > field, plus highWatermark, lastStableOffset, logStartOffset, etc. But when
> > the "error" field is set, those other fields are not used. Perhaps we
> > could save some space by just having a separate array of "partitions with
> > errors." In the common case where there are no errors, this would save 2
> > bytes per partition, which could be quite significant in large responses.
> >
> > best,
> > Colin
> >
> > >
> > > Jun
> > >
> > >
> > > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <cm...@apache.org>
> > wrote:
> > >
> > > > Hi all,
> > > >
> > > > I updated the KIP. There is also an implementation of this KIP here:
> > > > https://github.com/apache/kafka/pull/4418
> > > >
> > > > The updated implementation simplifies a few things, and adds the
> > ability
> > > > to incrementally add or remove individual partitions in an incremental
> > > > fetch request.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > > Hi all,
> > > > >
> > > > > I'd like to start the vote on KIP-227: Incremental Fetch Requests.
> > > > >
> > > > > The KIP is here:
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> > > > Partition+Scalability
> > > > >
> > > > > and discussion thread earlier:
> > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.html
> > > > >
> > > > > thanks,
> > > > > Colin
> > > >
> >
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Jason Gustafson <ja...@confluent.io>.
Hi Colin,
This is looking good to me. A few comments:
1. The fetch type seems unnecessary in the request and response schemas
since it can be inferred by the sessionId/epoch.
2. I agree with Jun that a separate array for partitions to remove would be
more intuitive.
3. I'm not super thrilled with the cache configuration since it seems to
tie us a bit too closely to the implementation. You've mostly convinced me
on the need for the slots config, but I wonder if we can at least do
without "min.incremental.fetch.session.eviction.ms"? For one, I think the
broker should reserve the right to evict sessions at will. We shouldn't be
stuck maintaining a small session at the expense of a much larger one just
to enforce this timeout. Internally, I think having some cache stickiness
to avoid thrashing makes sense, but I think static values are likely to be
good enough and that lets us retain some flexibility to change the behavior
in the future.
4. I think the word "incremental" is redundant in the config names. Maybe
it could just be "max.fetch.session.cache.slots" for example?
Thanks,
Jason
On Sat, Jan 20, 2018 at 12:54 PM, Colin McCabe <cm...@apache.org> wrote:
> On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> > Hi, Colin,
> >
> > Thanks for the KIP. Looks good to me overall. Just a couple of more
> > comments.
> >
> > 1. As I mentioned earlier, it might be useful to add some metrics for
> > monitoring the usage of the session cache. For example, it would be
> useful
> > to know how many slots are being used (or unused), # of total partitions
> in
> > the cached slots (to understand space), the eviction rate (to see if
> there
> > is any churn), etc.
>
> Thanks, Jun. Sorry-- I meant to address this earlier, but I forgot about
> it. I just added some proposed metrics to the KIP wiki.
>
> >
> > 2. Using max_bytes to 0 represent the removal of a partition seems
> > unintuitive. Perhaps it's better to either add a flag per partition or
> add
> > a removed partition list.
>
> Perhaps if we use max_bytes -1 to represent removal, it will be more
> intuitive? After all, -1 bytes is clearly not a valid amount of bytes to
> fetch. Or should be have a separate array of removed TopicPartitions?
>
> On a related note, in the FetchResponse#PartitionData, we have an "error"
> field, plus highWatermark, lastStableOffset, logStartOffset, etc. But when
> the "error" field is set, those other fields are not used. Perhaps we
> could save some space by just having a separate array of "partitions with
> errors." In the common case where there are no errors, this would save 2
> bytes per partition, which could be quite significant in large responses.
>
> best,
> Colin
>
> >
> > Jun
> >
> >
> > On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <cm...@apache.org>
> wrote:
> >
> > > Hi all,
> > >
> > > I updated the KIP. There is also an implementation of this KIP here:
> > > https://github.com/apache/kafka/pull/4418
> > >
> > > The updated implementation simplifies a few things, and adds the
> ability
> > > to incrementally add or remove individual partitions in an incremental
> > > fetch request.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > > Hi all,
> > > >
> > > > I'd like to start the vote on KIP-227: Incremental Fetch Requests.
> > > >
> > > > The KIP is here:
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> > > Partition+Scalability
> > > >
> > > > and discussion thread earlier:
> > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.html
> > > >
> > > > thanks,
> > > > Colin
> > >
>
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Colin McCabe <cm...@apache.org>.
On Fri, Jan 19, 2018, at 15:02, Jun Rao wrote:
> Hi, Colin,
>
> Thanks for the KIP. Looks good to me overall. Just a couple of more
> comments.
>
> 1. As I mentioned earlier, it might be useful to add some metrics for
> monitoring the usage of the session cache. For example, it would be useful
> to know how many slots are being used (or unused), # of total partitions in
> the cached slots (to understand space), the eviction rate (to see if there
> is any churn), etc.
Thanks, Jun. Sorry-- I meant to address this earlier, but I forgot about it. I just added some proposed metrics to the KIP wiki.
>
> 2. Using max_bytes to 0 represent the removal of a partition seems
> unintuitive. Perhaps it's better to either add a flag per partition or add
> a removed partition list.
Perhaps if we use max_bytes -1 to represent removal, it will be more intuitive? After all, -1 bytes is clearly not a valid amount of bytes to fetch. Or should be have a separate array of removed TopicPartitions?
On a related note, in the FetchResponse#PartitionData, we have an "error" field, plus highWatermark, lastStableOffset, logStartOffset, etc. But when the "error" field is set, those other fields are not used. Perhaps we could save some space by just having a separate array of "partitions with errors." In the common case where there are no errors, this would save 2 bytes per partition, which could be quite significant in large responses.
best,
Colin
>
> Jun
>
>
> On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <cm...@apache.org> wrote:
>
> > Hi all,
> >
> > I updated the KIP. There is also an implementation of this KIP here:
> > https://github.com/apache/kafka/pull/4418
> >
> > The updated implementation simplifies a few things, and adds the ability
> > to incrementally add or remove individual partitions in an incremental
> > fetch request.
> >
> > best,
> > Colin
> >
> >
> > On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > > Hi all,
> > >
> > > I'd like to start the vote on KIP-227: Incremental Fetch Requests.
> > >
> > > The KIP is here:
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> > Partition+Scalability
> > >
> > > and discussion thread earlier:
> > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.html
> > >
> > > thanks,
> > > Colin
> >
Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to
Increase Partition Scalability
Posted by Jun Rao <ju...@confluent.io>.
Hi, Colin,
Thanks for the KIP. Looks good to me overall. Just a couple of more
comments.
1. As I mentioned earlier, it might be useful to add some metrics for
monitoring the usage of the session cache. For example, it would be useful
to know how many slots are being used (or unused), # of total partitions in
the cached slots (to understand space), the eviction rate (to see if there
is any churn), etc.
2. Using max_bytes to 0 represent the removal of a partition seems
unintuitive. Perhaps it's better to either add a flag per partition or add
a removed partition list.
Jun
On Thu, Jan 18, 2018 at 6:15 PM, Colin McCabe <cm...@apache.org> wrote:
> Hi all,
>
> I updated the KIP. There is also an implementation of this KIP here:
> https://github.com/apache/kafka/pull/4418
>
> The updated implementation simplifies a few things, and adds the ability
> to incrementally add or remove individual partitions in an incremental
> fetch request.
>
> best,
> Colin
>
>
> On Tue, Dec 19, 2017, at 19:28, Colin McCabe wrote:
> > Hi all,
> >
> > I'd like to start the vote on KIP-227: Incremental Fetch Requests.
> >
> > The KIP is here:
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 227%3A+Introduce+Incremental+FetchRequests+to+Increase+
> Partition+Scalability
> >
> > and discussion thread earlier:
> > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.html
> >
> > thanks,
> > Colin
>