Hi, Andrey, Thanks for the reply. A couple of more comments inline below.
On Wed, Aug 10, 2016 at 3:56 AM, Andrey L. Neporada < anepor...@yandex-team.ru> wrote: > Hi! > > > On 09 Aug 2016, at 20:46, Jun Rao <j...@confluent.io> wrote: > > > > Hi, Andrey, > > > > Thanks for the proposal. It looks good overall. Some minor comments. > > > > 1. It seems that it's bit weird that fetch.partition.max.bytes is a > broker > > level configuration while fetch.limit.bytes is a client side > configuration. > > Intuitively, it seems both should be set by the client? If we do that, > one > > benefit is that we can validate that fetch.limit.bytes >= > > fetch.partition.max.bytes on the client side. > > > > Yes, such cooperative configuration for fetch request may look a bit weird. > But I don’t see other options if we want to remove partition limits from > fetch request. > In this case we need some server-side configuration for partition limits. > > What if we keep the current partition level limit in the fetch request and just add an additional response level limit? The default partition limit can be much smaller than the max message size and will only be used for fairness across partitions. > > 2. Naming wise. fetch.response.max.bytes and replica.fetch.response.max. > bytes > > seem to be more consistent with our current convention than > > fetch.limit.bytes and replica.fetch.limit.bytes. > > Agree, will rename. > > > > > 3. When you say "This way we can ensure that response size is less than ( > > *limit_bytes* + *message.max.bytes*).", it should be "less than > > max(limit_bytes, message.max.bytes)", right? > > > > No, I mean that actual response side can be bigger than limit_bytes, but > less than limit_bytes + message.max.bytes. > This behaviour is a result of algorithm proposed in KIP (and in PR). > > Got it. An alternative is to only add a partition's data to the response up to the remaining response limit. The only exception is that this is the first partition and the first message in that partition is larger than the response limit. Then the bound will be max(limit_bytes, message.max.bytes), which is tighter. > > Finally, KIP-73 (replication quota) is proposing a similar change to > fetch > > request protocol. We can probably just combine the two changes into one, > > instead of bumping the fetch request version twice. > > Fine with that. > > > > > Thanks, > > > > Jun > > > > Thanks, > Andrey. > >
