----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52403/#review150909 -----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 65) <https://reviews.apache.org/r/52403/#comment218979> maybe firstCallbackException? Don't think we need @volatile anymore. Minor: Don't need type declaration. Minor: update documentation. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 117) <https://reviews.apache.org/r/52403/#comment218980> Can you explain/document what we're trying to achieve here? Is this producer meant to somehow heal itself eventually and stop throwing this exception? If that's the case doesn't that mean that "callbackExceptionFirstThrown" is a misnomer - it isn't the exception first thrown anymore. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 140) <https://reviews.apache.org/r/52403/#comment218985> Unrelated to this change: Strange formatting, move to previous line or use braces. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 152) <https://reviews.apache.org/r/52403/#comment218986> What does this retry refer to? Looks like we just close the producer in case of failure? samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 156) <https://reviews.apache.org/r/52403/#comment218983> Minor: use logger.error for consistency. Minor: capitalize "Closing" samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (lines 157 - 158) <https://reviews.apache.org/r/52403/#comment218988> If the producer is meant to heal itself eventually, is this still true? samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 162) <https://reviews.apache.org/r/52403/#comment218984> Curious why we use format instead of Scala interpolated strings? samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 202) <https://reviews.apache.org/r/52403/#comment218982> Wondering why we rethrow a previously saved exception instead of no-oping the flush? Is the user expected to handle this? If so, should document why & how. - Prateek Maheshwari On Sept. 29, 2016, 12:04 p.m., Xinyu Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52403/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2016, 12:04 p.m.) > > > Review request for samza, Navina Ramesh and Jagadish Venkatraman. > > > Repository: samza > > > Description > ------- > > Current the error log happens after produce close and reset the exception in > later callbacks, which caused the trouble shooting to be harder in cases of > multithreading. We should log error before closing and keep atomic reference > of the initial exception. > > > Diffs > ----- > > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala > 5ff6d3caf54ed148aa40c7c752c587e556a4f34a > > Diff: https://reviews.apache.org/r/52403/diff/ > > > Testing > ------- > > Tested in jobs deployed in Yarn cluster. > > > Thanks, > > Xinyu Liu > >
