kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Colin McCabe <cmcc...@apache.org>
Subject Re: [VOTE] KIP-227: Introduce Fetch Requests that are Incremental to Increase Partition Scalability
Date Mon, 22 Jan 2018 23:56:40 GMT
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
> > > >
> >

Mime
View raw message