You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2015/01/27 17:16:34 UTC
Review Request 30321: Patch for kafka-1902
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/
-----------------------------------------------------------
Review request for kafka.
Bugs: kafka-1902
https://issues.apache.org/jira/browse/kafka-1902
Repository: kafka
Description
-------
fix metric name by adding tags as scope
Diffs
-----
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
Diff: https://reviews.apache.org/r/30321/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 30321: Patch for kafka-1902
Posted by Eric Olander <ol...@gmail.com>.
> On Jan. 28, 2015, 2:34 a.m., Eric Olander wrote:
> > core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala, line 66
> > <https://reviews.apache.org/r/30321/diff/1/?file=836389#file836389line66>
> >
> > val scope = tagsName.map {t => nameBuilder.append(",").append(t)}.orNull
>
> Eric Olander wrote:
> Sorry - I have a typo in the above comment
> val scope = tagsName.map {t => nameBuilder.append(",").append(t); t}.orNull
Alright - sorry to keep churning on this. Things are always clearer in the morning. Really there are two things happening here that are being handled though side effects - assignment of the scope var and updating nameBuilder. I'd prefer to use less of the Java style and move more to the Scala way of handling things like this. So,
val scope = tagsName.orNull
That will preserve the original semantics of what you did - scope is either the contents of the Some or null.
tagsName.foreach {t => nameBuilder.append(",").append(t)}
Side-effecting code on collections is typically performed within a foreach so this makes things more apparent.
- Eric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/#review69940
-----------------------------------------------------------
On Jan. 27, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30321/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1902
> https://issues.apache.org/jira/browse/kafka-1902
>
>
> Repository: kafka
>
>
> Description
> -------
>
> fix metric name by adding tags as scope
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
>
> Diff: https://reviews.apache.org/r/30321/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 30321: Patch for kafka-1902
Posted by Eric Olander <ol...@gmail.com>.
> On Jan. 28, 2015, 2:34 a.m., Eric Olander wrote:
> > core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala, line 66
> > <https://reviews.apache.org/r/30321/diff/1/?file=836389#file836389line66>
> >
> > val scope = tagsName.map {t => nameBuilder.append(",").append(t)}.orNull
Sorry - I have a typo in the above comment
val scope = tagsName.map {t => nameBuilder.append(",").append(t); t}.orNull
- Eric
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/#review69940
-----------------------------------------------------------
On Jan. 27, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30321/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1902
> https://issues.apache.org/jira/browse/kafka-1902
>
>
> Repository: kafka
>
>
> Description
> -------
>
> fix metric name by adding tags as scope
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
>
> Diff: https://reviews.apache.org/r/30321/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 30321: Patch for kafka-1902
Posted by Eric Olander <ol...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/#review69940
-----------------------------------------------------------
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala
<https://reviews.apache.org/r/30321/#comment114778>
val scope = tagsName.map {t => nameBuilder.append(",").append(t)}.orNull
- Eric Olander
On Jan. 27, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30321/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1902
> https://issues.apache.org/jira/browse/kafka-1902
>
>
> Repository: kafka
>
>
> Description
> -------
>
> fix metric name by adding tags as scope
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
>
> Diff: https://reviews.apache.org/r/30321/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 30321: Patch for kafka-1902
Posted by Jun Rao <ju...@gmail.com>.
> On Jan. 27, 2015, 4:44 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala, line 69
> > <https://reviews.apache.org/r/30321/diff/1/?file=836389#file836389line69>
> >
> > Seems when yammer creates the mBeanName, it will use append ",scope=" before scope string, so I am wondering if it is OK to have "=" inside scope string?
> >
> > http://grepcode.com/file/repo1.maven.org/maven2/com.yammer.metrics/metrics-core/2.1.1/com/yammer/metrics/core/MetricName.java#MetricName.createMBeanName%28java.lang.String%2Cjava.lang.String%2Cjava.lang.String%2Cjava.lang.String%29
In our usage, we create the mbeanName ourselves and pass it into MetricName. So, we are not using the default mbeanName convention.
- Jun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/#review69814
-----------------------------------------------------------
On Jan. 27, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30321/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1902
> https://issues.apache.org/jira/browse/kafka-1902
>
>
> Repository: kafka
>
>
> Description
> -------
>
> fix metric name by adding tags as scope
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
>
> Diff: https://reviews.apache.org/r/30321/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 30321: Patch for kafka-1902
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/#review69814
-----------------------------------------------------------
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala
<https://reviews.apache.org/r/30321/#comment114617>
Seems when yammer creates the mBeanName, it will use append ",scope=" before scope string, so I am wondering if it is OK to have "=" inside scope string?
http://grepcode.com/file/repo1.maven.org/maven2/com.yammer.metrics/metrics-core/2.1.1/com/yammer/metrics/core/MetricName.java#MetricName.createMBeanName%28java.lang.String%2Cjava.lang.String%2Cjava.lang.String%2Cjava.lang.String%29
- Guozhang Wang
On Jan. 27, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30321/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1902
> https://issues.apache.org/jira/browse/kafka-1902
>
>
> Repository: kafka
>
>
> Description
> -------
>
> fix metric name by adding tags as scope
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
>
> Diff: https://reviews.apache.org/r/30321/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 30321: Patch for kafka-1902
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/#review70031
-----------------------------------------------------------
Ship it!
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala
<https://reviews.apache.org/r/30321/#comment114962>
I thought the earlier idiom was better - i.e., option.map{...}
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala
<https://reviews.apache.org/r/30321/#comment114963>
can be on same line?
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala
<https://reviews.apache.org/r/30321/#comment114965>
since some reporters (such as graphite)
I don't think jmx reporter cares about dot (or does it?)
- Joel Koshy
On Jan. 28, 2015, 5:23 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30321/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2015, 5:23 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1902
> https://issues.apache.org/jira/browse/kafka-1902
>
>
> Repository: kafka
>
>
> Description
> -------
>
> replace dot with $ in scope
>
>
> use the dot convention in scope
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
>
> Diff: https://reviews.apache.org/r/30321/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>
Re: Review Request 30321: Patch for kafka-1902
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/
-----------------------------------------------------------
(Updated Jan. 28, 2015, 5:23 p.m.)
Review request for kafka.
Bugs: kafka-1902
https://issues.apache.org/jira/browse/kafka-1902
Repository: kafka
Description (updated)
-------
replace dot with $ in scope
use the dot convention in scope
Diffs (updated)
-----
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
Diff: https://reviews.apache.org/r/30321/diff/
Testing
-------
Thanks,
Jun Rao
Re: Review Request 30321: Patch for kafka-1902
Posted by Manikumar Reddy O <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30321/#review69813
-----------------------------------------------------------
Ship it!
Ship It!
- Manikumar Reddy O
On Jan. 27, 2015, 4:16 p.m., Jun Rao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30321/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2015, 4:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: kafka-1902
> https://issues.apache.org/jira/browse/kafka-1902
>
>
> Repository: kafka
>
>
> Description
> -------
>
> fix metric name by adding tags as scope
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala e9e49180f6de45f98e79374f519f6097b4fc8637
>
> Diff: https://reviews.apache.org/r/30321/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jun Rao
>
>