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