You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by David Chen <dc...@linkedin.com> on 2014/09/16 02:53:57 UTC
Review Request 25677: SAMZA-407: Add metric for counting exceptions
by type
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/
-----------------------------------------------------------
Review request for samza.
Bugs: SAMZA-407
https://issues.apache.org/jira/browse/SAMZA-407
Repository: samza
Description
-------
SAMZA-407: Add metric for counting exceptions by type
Diffs
-----
samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
samza-api/src/main/java/org/apache/samza/util/BlockingEnvelopeMap.java 317e073485e4b8f37305aa551425cfa54e0c98bb
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala 7d9ff0040ff75a49e21d26d28d8940e5328fb2c5
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 8a04a8ad734439e4b7de24f088fc3c064346ab34
samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala be53373d143c2d504824ee1251f978ffefe31a33
samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
samza-shell/src/main/bash/run-am.sh c2025965c6c6199ad6879b1da3b9dc68e4f599f1
samza-shell/src/main/bash/run-class.sh 147396563c2f5924bbeecb84b68c92323e9235db
samza-shell/src/main/bash/run-container.sh 72cee188f956794df9608823c0bb188b544a8e1a
Diff: https://reviews.apache.org/r/25677/diff/
Testing
-------
Unit tests pass
Thanks,
David Chen
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by David Chen <dc...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/
-----------------------------------------------------------
(Updated Sept. 17, 2014, 10:18 p.m.)
Review request for samza.
Bugs: SAMZA-407
https://issues.apache.org/jira/browse/SAMZA-407
Repository: samza
Description
-------
SAMZA-407: Add metric for counting exceptions by type
Diffs (updated)
-----
docs/learn/documentation/versioned/jobs/configuration-table.html 526ca9ff43534e1cc82f76bbd60c00745803db28
samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala PRE-CREATION
samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala b7a9569de78ba5e794a05e336421588f5e197804
samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala c31a74e60ceeefb67bab92a6c39cc39184d6e3e6
samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
Diff: https://reviews.apache.org/r/25677/diff/
Testing
-------
Unit tests pass
Thanks,
David Chen
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by David Chen <dc...@linkedin.com>.
> On Sept. 17, 2014, 4:42 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala, line 41
> > <https://reviews.apache.org/r/25677/diff/4/?file=691315#file691315line41>
> >
> > Should use new TaskInstanceMetrics here.
Oops. Missed this. Fixed.
> On Sept. 17, 2014, 4:42 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala, line 63
> > <https://reviews.apache.org/r/25677/diff/4/?file=691314#file691314line63>
> >
> > Just a nit, but it's legal and a bit more succinct to do:
> >
> > TaskInstanceExceptionHandler(metrics, config)
> >
> > Also, seems like we might want exceptionHandler in the constructor, so it's injectible for tests.
Done.
> On Sept. 17, 2014, 4:42 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala, line 73
> > <https://reviews.apache.org/r/25677/diff/4/?file=691315#file691315line73>
> >
> > Either "Counting exception " + className, or debug("Counting exception", exception).
Fixed.
> On Sept. 17, 2014, 4:42 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala, line 78
> > <https://reviews.apache.org/r/25677/diff/4/?file=691315#file691315line78>
> >
> > Need a more descriptive counter name here. The metric we'd get out right now would just be "com.foo.bar.MyException".
> >
> > Some sort of prefix like "exception-ignored-%s" format className, would be useful, I think.
I have added the "exception-ignored-" prefix.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/#review53689
-----------------------------------------------------------
On Sept. 17, 2014, 12:42 a.m., David Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25677/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 12:42 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-407
> https://issues.apache.org/jira/browse/SAMZA-407
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-407: Add metric for counting exceptions by type
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/jobs/configuration-table.html 526ca9ff43534e1cc82f76bbd60c00745803db28
> samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
> samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
> samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala PRE-CREATION
> samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala b7a9569de78ba5e794a05e336421588f5e197804
> samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala c31a74e60ceeefb67bab92a6c39cc39184d6e3e6
> samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
>
> Diff: https://reviews.apache.org/r/25677/diff/
>
>
> Testing
> -------
>
> Unit tests pass
>
>
> Thanks,
>
> David Chen
>
>
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/#review53689
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala
<https://reviews.apache.org/r/25677/#comment93477>
Just a nit, but it's legal and a bit more succinct to do:
TaskInstanceExceptionHandler(metrics, config)
Also, seems like we might want exceptionHandler in the constructor, so it's injectible for tests.
samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala
<https://reviews.apache.org/r/25677/#comment93478>
Should use new TaskInstanceMetrics here.
samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala
<https://reviews.apache.org/r/25677/#comment93480>
Either "Counting exception " + className, or debug("Counting exception", exception).
samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala
<https://reviews.apache.org/r/25677/#comment93479>
Need a more descriptive counter name here. The metric we'd get out right now would just be "com.foo.bar.MyException".
Some sort of prefix like "exception-ignored-%s" format className, would be useful, I think.
- Chris Riccomini
On Sept. 17, 2014, 12:42 a.m., David Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25677/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 12:42 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-407
> https://issues.apache.org/jira/browse/SAMZA-407
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-407: Add metric for counting exceptions by type
>
>
> Diffs
> -----
>
> docs/learn/documentation/versioned/jobs/configuration-table.html 526ca9ff43534e1cc82f76bbd60c00745803db28
> samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
> samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
> samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala PRE-CREATION
> samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala b7a9569de78ba5e794a05e336421588f5e197804
> samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala c31a74e60ceeefb67bab92a6c39cc39184d6e3e6
> samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
>
> Diff: https://reviews.apache.org/r/25677/diff/
>
>
> Testing
> -------
>
> Unit tests pass
>
>
> Thanks,
>
> David Chen
>
>
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by David Chen <dc...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/
-----------------------------------------------------------
(Updated Sept. 17, 2014, 12:42 a.m.)
Review request for samza.
Changes
-------
Address code review comments.
Bugs: SAMZA-407
https://issues.apache.org/jira/browse/SAMZA-407
Repository: samza
Description
-------
SAMZA-407: Add metric for counting exceptions by type
Diffs (updated)
-----
docs/learn/documentation/versioned/jobs/configuration-table.html 526ca9ff43534e1cc82f76bbd60c00745803db28
samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
samza-core/src/main/scala/org/apache/samza/container/TaskInstanceExceptionHandler.scala PRE-CREATION
samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala b7a9569de78ba5e794a05e336421588f5e197804
samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala c31a74e60ceeefb67bab92a6c39cc39184d6e3e6
samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
Diff: https://reviews.apache.org/r/25677/diff/
Testing
-------
Unit tests pass
Thanks,
David Chen
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by David Chen <dc...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 8:01 p.m.)
Review request for samza.
Bugs: SAMZA-407
https://issues.apache.org/jira/browse/SAMZA-407
Repository: samza
Description
-------
SAMZA-407: Add metric for counting exceptions by type
Diffs (updated)
-----
samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala 7d9ff0040ff75a49e21d26d28d8940e5328fb2c5
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala b7a9569de78ba5e794a05e336421588f5e197804
samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala c31a74e60ceeefb67bab92a6c39cc39184d6e3e6
samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
Diff: https://reviews.apache.org/r/25677/diff/
Testing
-------
Unit tests pass
Thanks,
David Chen
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by David Chen <dc...@linkedin.com>.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala, line 38
> > <https://reviews.apache.org/r/25677/diff/2/?file=690206#file690206line38>
> >
> > Add to config-table.html
Done.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala, line 34
> > <https://reviews.apache.org/r/25677/diff/2/?file=690207#file690207line34>
> >
> > Should add debug logging here, as well. Debug message should include exception (debug("msg", e))
Done.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala, line 134
> > <https://reviews.apache.org/r/25677/diff/2/?file=690210#file690210line134>
> >
> > Prefer having a lambda method in ExceptionCounts rather than full try/catch blocks here. Something like:
> >
> > exceptionCounts.maybeHandle {
> > task.proces(...)
> > }
Done.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala, line 39
> > <https://reviews.apache.org/r/25677/diff/2/?file=690207#file690207line39>
> >
> > Move this check above the counter increment. Otherwise, we might get an erroneous increment on a non-ignored exception, which could get reported via async metrics thread before the container is shut down.
> >
> > Might be nice to support a wild card here as well. A way for the dev to specify that they want to ignore any exception.
Done. I originally thought that exceptions that are not ignored should be counted as well.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala, line 20
> > <https://reviews.apache.org/r/25677/diff/2/?file=690207#file690207line20>
> >
> > Thinking this might be better located in util.
> >
> > Alternatively, if we want to peg it to be used for TaskInstance, I'd prefer keeping in this package, but naming it something like TaskInstanceExceptionHandler to keep with current naming hierarchy.
> >
> > Given that the wiring seems TaskInstance-specific (task.ignored.exceptions), it seems that the latter naming scheme is preferable. If we do this, then I believe that the MetricsHelper that the handler interacts with should be the TaskInstanceMetrics class, not the SamzaContainerMetrics class (see comments below for context).
Done. Since we are currently only interested in counting ignored exceptions in TaskInstances, I also agree that this class should be pegged to TaskInstance and that the exception metrics should live in TaskInstanceMetrics.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala, line 27
> > <https://reviews.apache.org/r/25677/diff/2/?file=690209#file690209line27>
> >
> > Remove (see above comment).
Done.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala, line 46
> > <https://reviews.apache.org/r/25677/diff/2/?file=690210#file690210line46>
> >
> > Default to = new ExceptionCounts(), and provide default constructor (also mentioned above) in ExceptionCounts.
Done.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala, line 41
> > <https://reviews.apache.org/r/25677/diff/2/?file=690209#file690209line41>
> >
> > I don't think that this will work. Most MetricsReporters don't know how to report a Gauge[Map[String, Int]]. Also, we want a counter, not a gauge for this metric.
> >
> > The alternative implementation (calling metricsHelper.newCounter from ExceptionCounter) that I've proposed above should work properly.
Done.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 143
> > <https://reviews.apache.org/r/25677/diff/2/?file=690208#file690208line143>
> >
> > I think this injection should be inversed.
> >
> > ExceptionCounts should take in a MetricsHelper, and should call newCounter() on it every time a new ignored exception comes in. It should then hold on to each counter, and increment it accordingly.
Agreed. Done.
> On Sept. 16, 2014, 3:47 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala, line 25
> > <https://reviews.apache.org/r/25677/diff/2/?file=690207#file690207line25>
> >
> > Java docs.
> >
> > Avoid Config constructors in Samza. Instead, provide an apply() companion method that does wiring. ExceptionCounts constructor should have actual parameters (i.e. ignoredExceptions: Set[String]). Default to an empty set, so we can have an empty constructor as well.
Done.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/#review53530
-----------------------------------------------------------
On Sept. 16, 2014, 8:01 p.m., David Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25677/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 8:01 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-407
> https://issues.apache.org/jira/browse/SAMZA-407
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-407: Add metric for counting exceptions by type
>
>
> Diffs
> -----
>
> samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
> samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala 7d9ff0040ff75a49e21d26d28d8940e5328fb2c5
> samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
> samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala b7a9569de78ba5e794a05e336421588f5e197804
> samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala c31a74e60ceeefb67bab92a6c39cc39184d6e3e6
> samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
>
> Diff: https://reviews.apache.org/r/25677/diff/
>
>
> Testing
> -------
>
> Unit tests pass
>
>
> Thanks,
>
> David Chen
>
>
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/#review53530
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala
<https://reviews.apache.org/r/25677/#comment93217>
Add to config-table.html
samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala
<https://reviews.apache.org/r/25677/#comment93226>
Thinking this might be better located in util.
Alternatively, if we want to peg it to be used for TaskInstance, I'd prefer keeping in this package, but naming it something like TaskInstanceExceptionHandler to keep with current naming hierarchy.
Given that the wiring seems TaskInstance-specific (task.ignored.exceptions), it seems that the latter naming scheme is preferable. If we do this, then I believe that the MetricsHelper that the handler interacts with should be the TaskInstanceMetrics class, not the SamzaContainerMetrics class (see comments below for context).
samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala
<https://reviews.apache.org/r/25677/#comment93219>
Java docs.
Avoid Config constructors in Samza. Instead, provide an apply() companion method that does wiring. ExceptionCounts constructor should have actual parameters (i.e. ignoredExceptions: Set[String]). Default to an empty set, so we can have an empty constructor as well.
samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala
<https://reviews.apache.org/r/25677/#comment93222>
Should add debug logging here, as well. Debug message should include exception (debug("msg", e))
samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala
<https://reviews.apache.org/r/25677/#comment93220>
Move this check above the counter increment. Otherwise, we might get an erroneous increment on a non-ignored exception, which could get reported via async metrics thread before the container is shut down.
Might be nice to support a wild card here as well. A way for the dev to specify that they want to ignore any exception.
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/25677/#comment93229>
I think this injection should be inversed.
ExceptionCounts should take in a MetricsHelper, and should call newCounter() on it every time a new ignored exception comes in. It should then hold on to each counter, and increment it accordingly.
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala
<https://reviews.apache.org/r/25677/#comment93230>
Remove (see above comment).
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala
<https://reviews.apache.org/r/25677/#comment93232>
I don't think that this will work. Most MetricsReporters don't know how to report a Gauge[Map[String, Int]]. Also, we want a counter, not a gauge for this metric.
The alternative implementation (calling metricsHelper.newCounter from ExceptionCounter) that I've proposed above should work properly.
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala
<https://reviews.apache.org/r/25677/#comment93233>
Default to = new ExceptionCounts(), and provide default constructor (also mentioned above) in ExceptionCounts.
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala
<https://reviews.apache.org/r/25677/#comment93234>
Prefer having a lambda method in ExceptionCounts rather than full try/catch blocks here. Something like:
exceptionCounts.maybeHandle {
task.proces(...)
}
- Chris Riccomini
On Sept. 16, 2014, 12:55 a.m., David Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25677/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2014, 12:55 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-407
> https://issues.apache.org/jira/browse/SAMZA-407
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-407: Add metric for counting exceptions by type
>
>
> Diffs
> -----
>
> samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
> samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala 7d9ff0040ff75a49e21d26d28d8940e5328fb2c5
> samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
> samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
> samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 8a04a8ad734439e4b7de24f088fc3c064346ab34
> samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala be53373d143c2d504824ee1251f978ffefe31a33
> samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
>
> Diff: https://reviews.apache.org/r/25677/diff/
>
>
> Testing
> -------
>
> Unit tests pass
>
>
> Thanks,
>
> David Chen
>
>
Re: Review Request 25677: SAMZA-407: Add metric for counting
exceptions by type
Posted by David Chen <dc...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25677/
-----------------------------------------------------------
(Updated Sept. 16, 2014, 12:55 a.m.)
Review request for samza.
Bugs: SAMZA-407
https://issues.apache.org/jira/browse/SAMZA-407
Repository: samza
Description
-------
SAMZA-407: Add metric for counting exceptions by type
Diffs (updated)
-----
samza-api/src/main/java/org/apache/samza/metrics/Gauge.java c37bfbbd6c0beb319e7b2af164f45689bde5b158
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 21d8903ccb1d80b48e54d72d391acbbeb7832b63
samza-core/src/main/scala/org/apache/samza/container/ExceptionCounts.scala PRE-CREATION
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 3288bf7dfaad81f28352d7c370eb72f984d3544b
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala 7d9ff0040ff75a49e21d26d28d8940e5328fb2c5
samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala b86fb0dfccbf0c179eb5aa77c4d4a15aa91cdf81
samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala ff425da2e76d9da2f37b0abb2941d6ffaa37b29b
samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala 8a04a8ad734439e4b7de24f088fc3c064346ab34
samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala be53373d143c2d504824ee1251f978ffefe31a33
samza-core/src/test/scala/org/apache/samza/metrics/TestMetricsHelper.scala 0a62bd001c60504cf566f5e59eb442ca8107befd
Diff: https://reviews.apache.org/r/25677/diff/
Testing
-------
Unit tests pass
Thanks,
David Chen