samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Riccomini" <criccom...@apache.org>
Subject Re: Review Request 25677: SAMZA-407: Add metric for counting exceptions by type
Date Tue, 16 Sep 2014 15:47:06 GMT

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


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