> On May 4, 2015, 5:18 p.m., Joel Koshy wrote: > > This looks good overall. Couple of questions: > > - You have blacklisted the topic for recording in the global broker topic > > metrics, but for consistency should we do this for per-topic metrics as > > well? > > - Rather than do the topic check in various places (which is error-prone) > > perhaps we can make BrokerTopicMetric accept a blacklist of topics > > (currently on offsets topic) and have setter methods (e.g., bytesIn, > > messagesOut, etc.) which will proceed to record the metric only if topic is > > not in blacklist. > > > > Another approach would be to maintain two sets of broker-topic-metrics. One > > for internal topics, one for other topics. The FailedProduce metrics should > > really have been called failed append.
Joel: Thanks much for reviewing my code. Please see my updated patch and reply below. - I thought we still need stats of offset topic while we don't want to include it in allTopicsStats. That is why I don't blacklist the topic for per-topoic metrics. What do you think? - I have added markBrokerTopicAndAllTopicsStats in BrokerTopicStats and substitute it to mark stats everywhere in the code. Does it look good? - Sure. I will make a pass of Kafka code to replace failedProduceRequest with failedAppendRequest and submit a patch, after previous changes pass the review. Thanks, - Dong ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33557/#review82392 ----------------------------------------------------------- On May 4, 2015, 10:18 p.m., Dong Lin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33557/ > ----------------------------------------------------------- > > (Updated May 4, 2015, 10:18 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1936 > https://issues.apache.org/jira/browse/KAFKA-1936 > > > Repository: kafka > > > Description > ------- > > KAFKA-1936; Track offset commit requests separately from producer requests > > > Diffs > ----- > > core/src/main/scala/kafka/log/Log.scala > 84e7b8fe9dd014884b60c4fbe13c835cf02a40e4 > core/src/main/scala/kafka/server/KafkaApis.scala > b4004aa3a1456d337199aa1245fb0ae61f6add46 > core/src/main/scala/kafka/server/KafkaRequestHandler.scala > a1558afed20bc651ca442a774920d782890167a5 > core/src/main/scala/kafka/server/ReplicaManager.scala > 59c9bc3ac3a8afc07a6f8c88c5871304db588d17 > core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala > 652208a70f66045b854549d93cbbc2b77c24b10b > > Diff: https://reviews.apache.org/r/33557/diff/ > > > Testing > ------- > > > Thanks, > > Dong Lin > >
