kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Colin McCabe <co...@cmccabe.xyz>
Subject Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to Increase Partition Scalability
Date Wed, 24 Jan 2018 06:17:41 GMT
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
> <cmccabe@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
> > > <jason@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
> > > > <cmccabe@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+Increas-
> > > > > > > > > e+> > > > > > > > >
Partition+Scalability
> > > > > > > > > >
> > > > > > > > > > and discussion thread earlier:
> > > > > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115>
> .
> > > > html
> > > > > > > > > >
> > > > > > > > > > thanks,
> > > > > > > > > > Colin
> > > > > > > > >
> > > > > > >
> > > > >
> > > >
> >


Mime
  • Unnamed multipart/alternative (inline, 7-Bit, 0 bytes)
View raw message