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