> On Sept. 29, 2016, 7:33 p.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, > > line 65 > > <https://reviews.apache.org/r/52403/diff/1/?file=1516363#file1516363line65> > > > > maybe firstCallbackException? > > Don't think we need @volatile anymore. > > > > Minor: Don't need type declaration. > > Minor: update documentation.
fixed. > On Sept. 29, 2016, 7:33 p.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, > > line 117 > > <https://reviews.apache.org/r/52403/diff/1/?file=1516363#file1516363line117> > > > > 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. This means we will throw the exception to the user, instead of failing container directly. We clear the exception since it's already reported. > On Sept. 29, 2016, 7:33 p.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, > > line 152 > > <https://reviews.apache.org/r/52403/diff/1/?file=1516363#file1516363line152> > > > > What does this retry refer to? Looks like we just close the producer in > > case of failure? This is legacy doc. Should be removed. fixed. > On Sept. 29, 2016, 7:33 p.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, > > line 156 > > <https://reviews.apache.org/r/52403/diff/1/?file=1516363#file1516363line156> > > > > Minor: use logger.error for consistency. > > Minor: capitalize "Closing" Fixed. > On Sept. 29, 2016, 7:33 p.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, > > lines 157-158 > > <https://reviews.apache.org/r/52403/diff/1/?file=1516363#file1516363line157> > > > > If the producer is meant to heal itself eventually, is this still true? I fixed the doc. Please take a look again. > On Sept. 29, 2016, 7:33 p.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, > > line 162 > > <https://reviews.apache.org/r/52403/diff/1/?file=1516363#file1516363line162> > > > > Curious why we use format instead of Scala interpolated strings? legacy logging. > On Sept. 29, 2016, 7:33 p.m., Prateek Maheshwari wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, > > line 202 > > <https://reviews.apache.org/r/52403/diff/1/?file=1516363#file1516363line202> > > > > 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 wrote: > Nvm this, I think the intention is to throw in the next operation after a > send error callback. I added the comments. Please take a look again. - Xinyu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52403/#review150909 ----------------------------------------------------------- On Sept. 29, 2016, 9 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, 9 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 > >
