Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Jagadish Venkatraman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/
---

(Updated May 11, 2016, 5:51 p.m.)


Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
Infrastructure).


Bugs: SAMZA-911
https://issues.apache.org/jira/browse/SAMZA-911


Repository: samza


Description (updated)
---

Currently, the KafkaSystemProducer's producer loop keeps retrying indefinitely 
when there is an exception in the retryBackOff loop. This is problematic 
because it will completely stall the Samza container (currently 
single-threaded). We've observed multiple jobs being affected as a result of 
this because of transient kafka-broker side errors.

If there are repeated exceptions, then it makes sense to retry for awhile, and 
then fail the container.

Long term fix: We should focus on getting rid off the retryBackOff loop, and 
close the producer object in the callback during failure. Closing the producer 
object in the callback-handler thread will guarantee in-order delivery. 
(SAMZA-934)

1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set to 
30).
2.Add tests to verify retry in case of RetriableExceptions.


Diffs
-

  
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 9a44d46d29a1997958a9d2bbf7be0bde860fff64 
  
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
 39426d8cf64516ec4fdc0cb4ff60b1df3a757470 

Diff: https://reviews.apache.org/r/45258/diff/


Testing
---

Added unit tests to verify functionality.


Thanks,

Jagadish Venkatraman

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Yi Pan (Data Infrastructure)]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review130847
---


Ship it!




Ship It!

- Yi Pan (Data Infrastructure)


On April 15, 2016, 1:32 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated April 15, 2016, 1:32 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Bugs: SAMZA-911
> https://issues.apache.org/jira/browse/SAMZA-911
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Yi Pan (Data Infrastructure)]

> On April 21, 2016, 5:21 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala,
> >  line 44
> > 
> >
> > It would be nice to add a producer configuration for the max retries.

Had a offline discussion w/ Jagadish and Navina. The issue of adding the config 
var is that in new KafkaProducer, this retry logic may not be needed at all 
(Kafka producer is internally doing the retries). Hence, the config var would 
be deprecated if were introduced now.


- Yi


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review129857
---


On April 15, 2016, 1:32 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated April 15, 2016, 1:32 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Bugs: SAMZA-911
> https://issues.apache.org/jira/browse/SAMZA-911
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Yi Pan (Data Infrastructure)]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review129857
---




samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 (line 44)


It would be nice to add a producer configuration for the max retries.


- Yi Pan (Data Infrastructure)


On April 15, 2016, 1:32 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated April 15, 2016, 1:32 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Bugs: SAMZA-911
> https://issues.apache.org/jira/browse/SAMZA-911
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Jagadish Venkatraman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/
---

(Updated April 15, 2016, 1:32 a.m.)


Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
Infrastructure).


Changes
---

Update according to Chris's feedback


Bugs: SAMZA-911
https://issues.apache.org/jira/browse/SAMZA-911


Repository: samza


Description
---

Currently, the KafkaSystemProducer's producer loop keeps retrying indefinitely 
when there is an exception in the retryBackOff loop. If there are repeated 
exceptions, then it makes sense to retry for awhile, and then fail the 
container.

Long term, we should focus on getting rid off the retryBackOff loop, and close 
the producer object in the callback during failure. This will guarantee 
in-order delivery.

1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set to 
30).
2.Add tests to verify retry in case of RetriableExceptions.


Diffs (updated)
-

  
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 9a44d46d29a1997958a9d2bbf7be0bde860fff64 
  
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
 39426d8cf64516ec4fdc0cb4ff60b1df3a757470 

Diff: https://reviews.apache.org/r/45258/diff/


Testing
---

Added unit tests to verify functionality.


Thanks,

Jagadish Venkatraman

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Jagadish Venkatraman]

> On April 14, 2016, 8:43 p.m., Chris Pettitt wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala,
> >  line 71
> > 
> >
> > This is not directly related to your commit, but:
> > 
> > This function is not reentrant. Is that expected? For example, it would 
> > be possible for a the Kafka callback to set sendFailed to true but a 
> > subsequent send would reset this flag.
> > 
> > 
> > ---
> > 
> > Separately, there is an implicit assumption that retryBackoff is 
> > providing a happens-before constraint between an invocation of the 
> > exception handler in retryBackoff and a subsequent invocation of the loop. 
> > This likely always holds, but a CAS for numRetries would cover cases where 
> > that does not hold.
> 
> Jagadish Venkatraman wrote:
> Yes, that's true. 
> 
> 1.It's entirely possible for a Kafka callback to set sendFailed to true, 
> and a subsequent send to reset this flag. Ideally, errors in the callbacks 
> should shut-down the producer in the same callback thread.
> 2. It's also possible for a subsequent send (for msg2) to be processed 
> even if the send for the previous message (msg1) fails.
> 
> This is the race in the producer that Navina mentioned in her review of 
> this RB. (and requested that a separate jira be created to track). SAMZA-934 
> tracks this.
> 
> Chris Pettitt wrote:
> Yes, my list was not exhaustive :). Fine to track the first issue via 
> JIRA. The second issue is introduced in this RB, tho.

I think the only thing that may fail the happens before constraint is: Part of 
the exception handler is updated in one thread, and the rest is updated in a 
separate thread. (we don't have this case now). I'll change it to an 
AtomicInteger instead of an integer.


- Jagadish


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review128972
---


On April 14, 2016, 10:05 p.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated April 14, 2016, 10:05 p.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Bugs: SAMZA-911
> https://issues.apache.org/jira/browse/SAMZA-911
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Chris Pettitt]

> On April 14, 2016, 8:43 p.m., Chris Pettitt wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala,
> >  line 71
> > 
> >
> > This is not directly related to your commit, but:
> > 
> > This function is not reentrant. Is that expected? For example, it would 
> > be possible for a the Kafka callback to set sendFailed to true but a 
> > subsequent send would reset this flag.
> > 
> > 
> > ---
> > 
> > Separately, there is an implicit assumption that retryBackoff is 
> > providing a happens-before constraint between an invocation of the 
> > exception handler in retryBackoff and a subsequent invocation of the loop. 
> > This likely always holds, but a CAS for numRetries would cover cases where 
> > that does not hold.
> 
> Jagadish Venkatraman wrote:
> Yes, that's true. 
> 
> 1.It's entirely possible for a Kafka callback to set sendFailed to true, 
> and a subsequent send to reset this flag. Ideally, errors in the callbacks 
> should shut-down the producer in the same callback thread.
> 2. It's also possible for a subsequent send (for msg2) to be processed 
> even if the send for the previous message (msg1) fails.
> 
> This is the race in the producer that Navina mentioned in her review of 
> this RB. (and requested that a separate jira be created to track). SAMZA-934 
> tracks this.

Yes, my list was not exhaustive :). Fine to track the first issue via JIRA. The 
second issue is introduced in this RB, tho.


- Chris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review128972
---


On April 14, 2016, 10:05 p.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated April 14, 2016, 10:05 p.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Bugs: SAMZA-911
> https://issues.apache.org/jira/browse/SAMZA-911
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Jagadish Venkatraman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/
---

(Updated April 14, 2016, 10:05 p.m.)


Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
Infrastructure).


Bugs: SAMZA-911
https://issues.apache.org/jira/browse/SAMZA-911


Repository: samza


Description
---

Currently, the KafkaSystemProducer's producer loop keeps retrying indefinitely 
when there is an exception in the retryBackOff loop. If there are repeated 
exceptions, then it makes sense to retry for awhile, and then fail the 
container.

Long term, we should focus on getting rid off the retryBackOff loop, and close 
the producer object in the callback during failure. This will guarantee 
in-order delivery.

1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set to 
30).
2.Add tests to verify retry in case of RetriableExceptions.


Diffs
-

  
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 9a44d46d29a1997958a9d2bbf7be0bde860fff64 
  
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
 39426d8cf64516ec4fdc0cb4ff60b1df3a757470 

Diff: https://reviews.apache.org/r/45258/diff/


Testing
---

Added unit tests to verify functionality.


Thanks,

Jagadish Venkatraman

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Jagadish Venkatraman]

> On April 14, 2016, 8:43 p.m., Chris Pettitt wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala,
> >  line 44
> > 
> >
> > Is 30 arbitrary or is there some significance to the number?

The choice of 30 was arbitrary. The goal is to abandon retrying after some 
number of retries. (if we keep getting exceptions due to broker side errors).


> On April 14, 2016, 8:43 p.m., Chris Pettitt wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala,
> >  line 71
> > 
> >
> > This is not directly related to your commit, but:
> > 
> > This function is not reentrant. Is that expected? For example, it would 
> > be possible for a the Kafka callback to set sendFailed to true but a 
> > subsequent send would reset this flag.
> > 
> > 
> > ---
> > 
> > Separately, there is an implicit assumption that retryBackoff is 
> > providing a happens-before constraint between an invocation of the 
> > exception handler in retryBackoff and a subsequent invocation of the loop. 
> > This likely always holds, but a CAS for numRetries would cover cases where 
> > that does not hold.

Yes, that's true. 

1.It's entirely possible for a Kafka callback to set sendFailed to true, and a 
subsequent send to reset this flag. Ideally, errors in the callbacks should 
shut-down the producer in the same callback thread.
2. It's also possible for a subsequent send (for msg2) to be processed even if 
the send for the previous message (msg1) fails.

This is the race in the producer that Navina mentioned in her review of this 
RB. (and requested that a separate jira be created to track). SAMZA-934 tracks 
this.


- Jagadish


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review128972
---


On March 24, 2016, 1:10 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Chris Pettitt]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review128972
---




samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 (line 44)


Is 30 arbitrary or is there some significance to the number?



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 (line 71)


This is not directly related to your commit, but:

This function is not reentrant. Is that expected? For example, it would be 
possible for a the Kafka callback to set sendFailed to true but a subsequent 
send would reset this flag.

---

Separately, there is an implicit assumption that retryBackoff is providing 
a happens-before constraint between an invocation of the exception handler in 
retryBackoff and a subsequent invocation of the loop. This likely always holds, 
but a CAS for numRetries would cover cases where that does not hold.


- Chris Pettitt


On March 24, 2016, 1:10 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Boris Shkolnik]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review128924
---


Ship it!




Looks like 'emergency' Jira :).
'Fix-non-realted' nit. You should put jira numbers in the 'Bug' field of the 
review.

- Boris Shkolnik


On March 24, 2016, 1:10 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Jake Maes]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review128910
---


Ship it!




Ship It!

- Jake Maes


On March 24, 2016, 1:10 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Navina Ramesh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/#review125483
---


Ship it!




lgtm! +1 
On a side note, can you please document the existing race conditions with the 
producer in a separate JIRA? You can assign that to me. Thanks!

- Navina Ramesh


On March 24, 2016, 1:10 a.m., Jagadish Venkatraman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45258/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for samza, Jake Maes, Navina Ramesh, and Yi Pan (Data 
> Infrastructure).
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> Currently, the KafkaSystemProducer's producer loop keeps retrying 
> indefinitely when there is an exception in the retryBackOff loop. If there 
> are repeated exceptions, then it makes sense to retry for awhile, and then 
> fail the container.
> 
> Long term, we should focus on getting rid off the retryBackOff loop, and 
> close the producer object in the callback during failure. This will guarantee 
> in-order delivery.
> 
> 1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set 
> to 30).
> 2.Add tests to verify retry in case of RetriableExceptions.
> 
> 
> Diffs
> -
> 
>   
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
>  9a44d46d29a1997958a9d2bbf7be0bde860fff64 
>   
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
>  39426d8cf64516ec4fdc0cb4ff60b1df3a757470 
> 
> Diff: https://reviews.apache.org/r/45258/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests to verify functionality.
> 
> 
> Thanks,
> 
> Jagadish Venkatraman
> 
>

Re: Review Request 45258: Abandon producer retry after a certain # of errors : SAMZA-911

[Jagadish Venkatraman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45258/
---

(Updated March 24, 2016, 1:07 a.m.)


Review request for samza.


Summary (updated)
-

Abandon producer retry after a certain # of errors : SAMZA-911


Repository: samza


Description
---

Currently, the KafkaSystemProducer's producer loop keeps retrying indefinitely 
when there is an exception in the retryBackOff loop. If there are repeated 
exceptions, then it makes sense to retry for awhile, and then fail the 
container.

Long term, we should focus on getting rid off the retryBackOff loop, and close 
the producer object in the callback during failure. This will guarantee 
in-order delivery.

1.Modified the KafkaSystemProducer to take a maxRetries. (currently, its set to 
30).
2.Add tests to verify retry in case of RetriableExceptions.


Diffs
-

  
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
 9a44d46d29a1997958a9d2bbf7be0bde860fff64 
  
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala
 39426d8cf64516ec4fdc0cb4ff60b1df3a757470 

Diff: https://reviews.apache.org/r/45258/diff/


Testing
---

Added unit tests to verify functionality.


Thanks,

Jagadish Venkatraman