----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52403/#review150947 -----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 101) <https://reviews.apache.org/r/52403/#comment219040> private def. Also, maybe move private method below public methods? samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 106) <https://reviews.apache.org/r/52403/#comment219047> Nitpick: "Producer close ..." (capitalization) samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (lines 108 - 114) <https://reviews.apache.org/r/52403/#comment219046> Shouldn't this happen first? Otherwise a concurrent send could end up trying to use a closed producer. Also, my impression was that creating producers is expensive. What impact does recycling producers like this have on the kafka cluster and on the container resource usage (per producer overhead)? samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 165) <https://reviews.apache.org/r/52403/#comment219043> We can just remove this comment :) samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 170) <https://reviews.apache.org/r/52403/#comment219048> Comment redundant, same information in the line above. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 173) <https://reviews.apache.org/r/52403/#comment219049> "or producer.flush()" samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala (line 218) <https://reviews.apache.org/r/52403/#comment219042> If the intention is to allow user to ensure that the container doesn't die on producer failures, shouldn't this not throw either? If we end up flushing right after the callback exception, the user will not get a chance to ignore the exception. - Prateek Maheshwari On Sept. 29, 2016, 2:23 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, 2:23 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 > >
