You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Steven Yates <sy...@stevendyates.com> on 2014/02/04 13:33:43 UTC
Review Request 17705: SAMZA-96
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17705/
-----------------------------------------------------------
Review request for samza and Steven Yates.
Bugs: SAMZA-96
https://issues.apache.org/jira/browse/SAMZA-96
Repository: samza
Description
-------
Review request for SAMZA-96
Diffs
-----
samza-core/src/main/scala/org/apache/samza/util/DaemonThreadFactory.scala 04e67a2
samza-core/src/test/scala/org/apache/samza/util/TestDaemonThreadFactory.scala PRE-CREATION
Diff: https://reviews.apache.org/r/17705/diff/
Testing
-------
Unit test TestDaemonThreadFactory.scala
Thanks,
Steven Yates
Re: Review Request 17705: SAMZA-96
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17705/#review33614
-----------------------------------------------------------
This looks good.
Could you update MetricsSnapshotReporter and JvmMetrics to use the new naming method?
Could you also update BrokerProxy's thread to use the constant from DaemonThreadFactory, as well?
samza-core/src/main/scala/org/apache/samza/util/DaemonThreadFactory.scala
<https://reviews.apache.org/r/17705/#comment63150>
Prefer having a static constant defined in a DaemonThreadFactory object for thread prefix, and use constant prefix in unit test.
- Chris Riccomini
On Feb. 4, 2014, 12:33 p.m., Steven Yates wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17705/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 12:33 p.m.)
>
>
> Review request for samza and Steven Yates.
>
>
> Bugs: SAMZA-96
> https://issues.apache.org/jira/browse/SAMZA-96
>
>
> Repository: samza
>
>
> Description
> -------
>
> Review request for SAMZA-96
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/util/DaemonThreadFactory.scala 04e67a2
> samza-core/src/test/scala/org/apache/samza/util/TestDaemonThreadFactory.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/17705/diff/
>
>
> Testing
> -------
>
> Unit test TestDaemonThreadFactory.scala
>
>
> Thanks,
>
> Steven Yates
>
>
Re: Review Request 17705: SAMZA-96
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17705/#review33804
-----------------------------------------------------------
Ship it!
+1 LGTM
Thanks Steven!
- Chris Riccomini
On Feb. 6, 2014, 12:16 p.m., Steven Yates wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17705/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2014, 12:16 p.m.)
>
>
> Review request for samza and Steven Yates.
>
>
> Bugs: SAMZA-96
> https://issues.apache.org/jira/browse/SAMZA-96
>
>
> Repository: samza
>
>
> Description
> -------
>
> Review request for SAMZA-96
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/metrics/JvmMetrics.scala 301a5a0
> samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala 15af7aa
> samza-core/src/main/scala/org/apache/samza/util/DaemonThreadFactory.scala 04e67a2
> samza-core/src/test/scala/org/apache/samza/util/TestDaemonThreadFactory.scala PRE-CREATION
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 89730db
>
> Diff: https://reviews.apache.org/r/17705/diff/
>
>
> Testing
> -------
>
> Unit test TestDaemonThreadFactory.scala
>
>
> Thanks,
>
> Steven Yates
>
>
Re: Review Request 17705: SAMZA-96
Posted by Steven Yates <sy...@stevendyates.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17705/
-----------------------------------------------------------
(Updated Feb. 6, 2014, 12:16 p.m.)
Review request for samza and Steven Yates.
Changes
-------
Additional changes as discussed, thanks Chris
Bugs: SAMZA-96
https://issues.apache.org/jira/browse/SAMZA-96
Repository: samza
Description
-------
Review request for SAMZA-96
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/metrics/JvmMetrics.scala 301a5a0
samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala 15af7aa
samza-core/src/main/scala/org/apache/samza/util/DaemonThreadFactory.scala 04e67a2
samza-core/src/test/scala/org/apache/samza/util/TestDaemonThreadFactory.scala PRE-CREATION
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 89730db
Diff: https://reviews.apache.org/r/17705/diff/
Testing
-------
Unit test TestDaemonThreadFactory.scala
Thanks,
Steven Yates
Re: Review Request 17705: SAMZA-96
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17705/#review33703
-----------------------------------------------------------
Few minor nits. Also, could you post the patch to JIRA as well?
samza-core/src/main/scala/org/apache/samza/metrics/JvmMetrics.scala
<https://reviews.apache.org/r/17705/#comment63301>
Move outside the class, to top level object. Rename object to be a companion for JvmMetrics. That is, "object JvmMetrics { ..prefix constant }
Then do import MetricsSnapshotReporter._ in the class.
samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala
<https://reviews.apache.org/r/17705/#comment63300>
Move outside the class, to top level object. Rename object to be a companion for MetricsSnapshotReporter. That is, "object MetricsSnapshotReporter { ..prefix constant }
Then do import MetricsSnapshotReporter._ in the class.
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala
<https://reviews.apache.org/r/17705/#comment63299>
Prefer constant here, like other ones.
- Chris Riccomini
On Feb. 5, 2014, 11:38 a.m., Steven Yates wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17705/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2014, 11:38 a.m.)
>
>
> Review request for samza and Steven Yates.
>
>
> Bugs: SAMZA-96
> https://issues.apache.org/jira/browse/SAMZA-96
>
>
> Repository: samza
>
>
> Description
> -------
>
> Review request for SAMZA-96
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/metrics/JvmMetrics.scala 301a5a0
> samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala 15af7aa
> samza-core/src/main/scala/org/apache/samza/util/DaemonThreadFactory.scala 04e67a2
> samza-core/src/test/scala/org/apache/samza/util/TestDaemonThreadFactory.scala PRE-CREATION
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 89730db
>
> Diff: https://reviews.apache.org/r/17705/diff/
>
>
> Testing
> -------
>
> Unit test TestDaemonThreadFactory.scala
>
>
> Thanks,
>
> Steven Yates
>
>
Re: Review Request 17705: SAMZA-96
Posted by Steven Yates <sy...@stevendyates.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17705/
-----------------------------------------------------------
(Updated Feb. 5, 2014, 11:38 a.m.)
Review request for samza and Steven Yates.
Changes
-------
Let me know if this is what you had in mind Chris.
Bugs: SAMZA-96
https://issues.apache.org/jira/browse/SAMZA-96
Repository: samza
Description
-------
Review request for SAMZA-96
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/metrics/JvmMetrics.scala 301a5a0
samza-core/src/main/scala/org/apache/samza/metrics/reporter/MetricsSnapshotReporter.scala 15af7aa
samza-core/src/main/scala/org/apache/samza/util/DaemonThreadFactory.scala 04e67a2
samza-core/src/test/scala/org/apache/samza/util/TestDaemonThreadFactory.scala PRE-CREATION
samza-kafka/src/main/scala/org/apache/samza/system/kafka/BrokerProxy.scala 89730db
Diff: https://reviews.apache.org/r/17705/diff/
Testing
-------
Unit test TestDaemonThreadFactory.scala
Thanks,
Steven Yates