I'm late to the party but +1 and thanks for the KIP
On Thu, Jan 25, 2018 at 12:36 AM, Ismael Juma <ism...@juma.me.uk> wrote: > Agreed, Jun. > > Ismael > > On Wed, Jan 24, 2018 at 4:08 PM, Jun Rao <j...@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 >> > > <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+Increas- >> > > > > > > > > > > e+> > > > > > > > > Partition+Scalability >> > > > > > > > > > > > >> > > > > > > > > > > > and discussion thread earlier: >> > > > > > > > > > > > https://www.mail-archive.com/ >> > dev@kafka.apache.org/msg83115> > . >> > > > > > html >> > > > > > > > > > > > >> > > > > > > > > > > > thanks, >> > > > > > > > > > > > Colin >> > > > > > > > > > > >> > > > > > > > > >> > > > > > > >> > > > > > >> > > > >> > >> > >>
