samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Chen" <dc...@linkedin.com>
Subject Re: Review Request 25677: SAMZA-407: Add metric for counting exceptions by type
Date Wed, 17 Sep 2014 00:41:30 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message