> > 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+Increase+ > > > > > Partition+Scalability > > > > > > > > > > > > and discussion thread earlier: > > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg83115.html > > > > > > > > > > > > thanks, > > > > > > Colin > > > > > > > > >
