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 <cmcc...@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 > > > > > <cmcc...@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 > > > > > > > <cmcc...@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 < > > > cmcc...@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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
