You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Yan Fang <ya...@gmail.com> on 2015/03/17 10:37:25 UTC
Review Request 32155: SAMZA-458: Close in KafkaSystemProducer should
flush all source buffers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32155/
-----------------------------------------------------------
Review request for samza.
Bugs: SAMZA-458
https://issues.apache.org/jira/browse/SAMZA-458
Repository: samza
Description
-------
add flush in close method
only throw exceptions after flushing all sources
add unit test
Diffs
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala 83668dd
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala ca10ea5
Diff: https://reviews.apache.org/r/32155/diff/
Testing
-------
ran unit tests and integration test
Thanks,
Yan Fang
Re: Review Request 32155: SAMZA-458: Close in KafkaSystemProducer
should flush all source buffers
Posted by Yan Fang <ya...@gmail.com>.
> On March 18, 2015, 9:01 p.m., Navina Ramesh wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, line 49
> > <https://reviews.apache.org/r/32155/diff/1/?file=897641#file897641line49>
> >
> > Can you explain the purpose of "failedSources"?
It is used to record failed sources. Because maybe there are more than one source failed, we want to know all of them and only stop the producer after flushing all the sources. Does that make sense?
> On March 18, 2015, 9:01 p.m., Navina Ramesh wrote:
> > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala, line 148
> > <https://reviews.apache.org/r/32155/diff/1/?file=897641#file897641line148>
> >
> > Why can't you do the failedSources empty check here instead of Line 153 ?
This is because, if there are more than one sources, such as source1, source2. If source1 fails and source2 succeed, and we flush each of them. We do not close the producer after flushing source1, instead, we close the producer after flushing source2. When flushing source2, the sendFailed.get() is not run. That's why the stop is outside of the conditions.
- Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32155/#review76944
-----------------------------------------------------------
On March 17, 2015, 9:37 a.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32155/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 9:37 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-458
> https://issues.apache.org/jira/browse/SAMZA-458
>
>
> Repository: samza
>
>
> Description
> -------
>
> add flush in close method
> only throw exceptions after flushing all sources
> add unit test
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala 83668dd
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala ca10ea5
>
> Diff: https://reviews.apache.org/r/32155/diff/
>
>
> Testing
> -------
>
> ran unit tests and integration test
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 32155: SAMZA-458: Close in KafkaSystemProducer
should flush all source buffers
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32155/#review76944
-----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
<https://reviews.apache.org/r/32155/#comment124665>
Can you explain the purpose of "failedSources"?
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
<https://reviews.apache.org/r/32155/#comment124666>
Why can't you do the failedSources empty check here instead of Line 153 ?
- Navina Ramesh
On March 17, 2015, 9:37 a.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32155/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 9:37 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-458
> https://issues.apache.org/jira/browse/SAMZA-458
>
>
> Repository: samza
>
>
> Description
> -------
>
> add flush in close method
> only throw exceptions after flushing all sources
> add unit test
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala 83668dd
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala ca10ea5
>
> Diff: https://reviews.apache.org/r/32155/diff/
>
>
> Testing
> -------
>
> ran unit tests and integration test
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 32155: SAMZA-458: Close in KafkaSystemProducer
should flush all source buffers
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32155/#review77245
-----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala
<https://reviews.apache.org/r/32155/#comment125113>
@Yan, I have two questions here:
1. When flush on one source failed, why are we trying to continue w/ other sources? Isn't it true that we will anyways re-try the whole flush again when the producer is re-created?
2. In the code block from 153, wouldn't it be easier to set a noFlushStop flag s.t. the stop() method can avoid calling the flush again when stop() is triggered from failed flush anyways?
- Yi Pan (Data Infrastructure)
On March 17, 2015, 9:37 a.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32155/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 9:37 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-458
> https://issues.apache.org/jira/browse/SAMZA-458
>
>
> Repository: samza
>
>
> Description
> -------
>
> add flush in close method
> only throw exceptions after flushing all sources
> add unit test
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala 83668dd
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala ca10ea5
>
> Diff: https://reviews.apache.org/r/32155/diff/
>
>
> Testing
> -------
>
> ran unit tests and integration test
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 32155: SAMZA-458: Close in KafkaSystemProducer
should flush all source buffers
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32155/#review77282
-----------------------------------------------------------
Ship it!
Looks good to me.
- Yi Pan (Data Infrastructure)
On March 20, 2015, 9:40 p.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32155/
> -----------------------------------------------------------
>
> (Updated March 20, 2015, 9:40 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-458
> https://issues.apache.org/jira/browse/SAMZA-458
>
>
> Repository: samza
>
>
> Description
> -------
>
> add flush in close method
> only throw exceptions after flushing all sources
> add unit test
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala 83668dd
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala ca10ea5
>
> Diff: https://reviews.apache.org/r/32155/diff/
>
>
> Testing
> -------
>
> ran unit tests and integration test
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 32155: SAMZA-458: Close in KafkaSystemProducer
should flush all source buffers
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32155/#review77293
-----------------------------------------------------------
Ship it!
Ship It!
- Navina Ramesh
On March 20, 2015, 9:40 p.m., Yan Fang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32155/
> -----------------------------------------------------------
>
> (Updated March 20, 2015, 9:40 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-458
> https://issues.apache.org/jira/browse/SAMZA-458
>
>
> Repository: samza
>
>
> Description
> -------
>
> add flush in close method
> only throw exceptions after flushing all sources
> add unit test
>
>
> Diffs
> -----
>
> samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala 83668dd
> samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala ca10ea5
>
> Diff: https://reviews.apache.org/r/32155/diff/
>
>
> Testing
> -------
>
> ran unit tests and integration test
>
>
> Thanks,
>
> Yan Fang
>
>
Re: Review Request 32155: SAMZA-458: Close in KafkaSystemProducer
should flush all source buffers
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32155/
-----------------------------------------------------------
(Updated March 20, 2015, 9:40 p.m.)
Review request for samza.
Bugs: SAMZA-458
https://issues.apache.org/jira/browse/SAMZA-458
Repository: samza
Description
-------
add flush in close method
only throw exceptions after flushing all sources
add unit test
Diffs (updated)
-----
samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemProducer.scala 83668dd
samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemProducer.scala ca10ea5
Diff: https://reviews.apache.org/r/32155/diff/
Testing
-------
ran unit tests and integration test
Thanks,
Yan Fang