Hi Lucas, Sorry for the delay, just had a look at this. A couple of questions: - did you notice any positive change after implementing this KIP? I'm wondering if you have any experimental results that show the benefit of the two queues.
- priority is usually not sufficient in addressing the problem the KIP identifies. Even with priority queues, you will sometimes (often?) have the case that data plane requests will be ahead of the control plane requests. This happens because the system might have already started processing the data plane requests before the control plane ones arrived. So it would be good to know what % of the problem this KIP addresses. Thanks Eno On Fri, Jun 15, 2018 at 4:44 PM, Ted Yu <yuzhih...@gmail.com> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
