Change looks good. Thanks
On Fri, Jun 15, 2018 at 8:42 AM, Lucas Wang <lucasatu...@gmail.com> wrote: > Hi Ted, > > Thanks for the suggestion. I've updated the KIP. Please take another look. > > Lucas > > On Thu, Jun 14, 2018 at 6:34 PM, Ted Yu <yuzhih...@gmail.com> wrote: > > > Currently in KafkaConfig.scala : > > > > val QueuedMaxRequests = 500 > > > > It would be good if you can include the default value for this new config > > in the KIP. > > > > Thanks > > > > On Thu, Jun 14, 2018 at 4:28 PM, Lucas Wang <lucasatu...@gmail.com> > wrote: > > > > > Hi Ted, Dong > > > > > > I've updated the KIP by adding a new config, instead of reusing the > > > existing one. > > > Please take another look when you have time. Thanks a lot! > > > > > > Lucas > > > > > > On Thu, Jun 14, 2018 at 2:33 PM, Ted Yu <yuzhih...@gmail.com> wrote: > > > > > > > bq. that's a waste of resource if control request rate is low > > > > > > > > I don't know if control request rate can get to 100,000, likely not. > > Then > > > > using the same bound as that for data requests seems high. > > > > > > > > On Wed, Jun 13, 2018 at 10:13 PM, Lucas Wang <lucasatu...@gmail.com> > > > > wrote: > > > > > > > > > Hi Ted, > > > > > > > > > > Thanks for taking a look at this KIP. > > > > > Let's say today the setting of "queued.max.requests" in cluster A > is > > > > 1000, > > > > > while the setting in cluster B is 100,000. > > > > > The 100 times difference might have indicated that machines in > > cluster > > > B > > > > > have larger memory. > > > > > > > > > > By reusing the "queued.max.requests", the controlRequestQueue in > > > cluster > > > > B > > > > > automatically > > > > > gets a 100x capacity without explicitly bothering the operators. > > > > > I understand the counter argument can be that maybe that's a waste > of > > > > > resource if control request > > > > > rate is low and operators may want to fine tune the capacity of the > > > > > controlRequestQueue. > > > > > > > > > > I'm ok with either approach, and can change it if you or anyone > else > > > > feels > > > > > strong about adding the extra config. > > > > > > > > > > Thanks, > > > > > Lucas > > > > > > > > > > > > > > > On Wed, Jun 13, 2018 at 3:11 PM, Ted Yu <yuzhih...@gmail.com> > wrote: > > > > > > > > > > > Lucas: > > > > > > Under Rejected Alternatives, #2, can you elaborate a bit more on > > why > > > > the > > > > > > separate config has bigger impact ? > > > > > > > > > > > > Thanks > > > > > > > > > > > > On Wed, Jun 13, 2018 at 2:00 PM, Dong Lin <lindon...@gmail.com> > > > wrote: > > > > > > > > > > > > > Hey Luca, > > > > > > > > > > > > > > Thanks for the KIP. Looks good overall. Some comments below: > > > > > > > > > > > > > > - We usually specify the full mbean for the new metrics in the > > KIP. > > > > Can > > > > > > you > > > > > > > specify it in the Public Interface section similar to KIP-237 > > > > > > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > > 237%3A+More+Controller+Health+Metrics> > > > > > > > ? > > > > > > > > > > > > > > - Maybe we could follow the same pattern as KIP-153 > > > > > > > <https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > > 153%3A+Include+only+client+traffic+in+BytesOutPerSec+metric>, > > > > > > > where we keep the existing sensor name "BytesInPerSec" and add > a > > > new > > > > > > sensor > > > > > > > "ReplicationBytesInPerSec", rather than replacing the sensor > > name " > > > > > > > BytesInPerSec" with e.g. "ClientBytesInPerSec". > > > > > > > > > > > > > > - It seems that the KIP changes the semantics of the broker > > config > > > > > > > "queued.max.requests" because the number of total requests > queued > > > in > > > > > the > > > > > > > broker will be no longer bounded by "queued.max.requests". This > > > > > probably > > > > > > > needs to be specified in the Public Interfaces section for > > > > discussion. > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > Dong > > > > > > > > > > > > > > > > > > > > > On Wed, Jun 13, 2018 at 12:45 PM, Lucas Wang < > > > lucasatu...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi Kafka experts, > > > > > > > > > > > > > > > > I created KIP-291 to add a separate queue for controller > > > requests: > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-291% > > > > > > > > 3A+Have+separate+queues+for+control+requests+and+data+ > requests > > > > > > > > > > > > > > > > Can you please take a look and let me know your feedback? > > > > > > > > > > > > > > > > Thanks a lot for your time! > > > > > > > > Regards, > > > > > > > > Lucas > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
