You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <gu...@linkedin.com> on 2014/05/02 01:43:59 UTC
Review Request 20997: Fix KAFKA-1432
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1432
https://issues.apache.org/jira/browse/KAFKA-1432
Repository: kafka
Description
-------
Add new TransitProducer
KAFKA-1432.v1
Diffs
-----
core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
core/src/main/scala/kafka/producer/TransitProducer.scala PRE-CREATION
core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
Diff: https://reviews.apache.org/r/20997/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20997: Fix KAFKA-1432
Posted by Guozhang Wang <gu...@linkedin.com>.
> On May 2, 2014, 1:35 a.m., Jun Rao wrote:
> > perf/src/main/scala/kafka/perf/ProducerPerformance.scala, lines 308-309
> > <https://reviews.apache.org/r/20997/diff/1/?file=573270#file573270line308>
> >
> > Is there a reason that we change to send a null key? Will that break system tests?
In the original interface that value is actually used as the partition id instead of the key, and in the old producer it is hashed in the DefaultEventHandler, while for the new producer it is hashed before calling the send() call. With the null key passed in, the old producer will have sticky behavior while the new producer's round robin stays the same. I will convert the integer to byte array to make the producer also round robin.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review41998
-----------------------------------------------------------
On May 1, 2014, 11:46 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 11:46 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> 1. Add useNewProducer in the MirrorMaker.
> 2. Remove MirrorMaker's dependency on MigrationTool's producer thread and data channel.
> 3. Unify Old/New Producer in a new class for MirrorMaker, ProducerPerformance and ConsoleProducer
>
> Will remove kafka.tools.newproducer.MirrorMaker upon final commit.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/producer/TransitProducer.scala PRE-CREATION
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review41998
-----------------------------------------------------------
core/src/main/scala/kafka/producer/TransitProducer.scala
<https://reviews.apache.org/r/20997/#comment75743>
Could we call this sth like BaseProducer?
core/src/main/scala/kafka/tools/MirrorMaker.scala
<https://reviews.apache.org/r/20997/#comment75732>
Could the if test be inside map()?
core/src/main/scala/kafka/tools/MirrorMaker.scala
<https://reviews.apache.org/r/20997/#comment75734>
Why are we setting producerThreads to empty?
perf/src/main/scala/kafka/perf/ProducerPerformance.scala
<https://reviews.apache.org/r/20997/#comment75739>
Is there a reason that we change to send a null key? Will that break system tests?
- Jun Rao
On May 1, 2014, 11:46 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 11:46 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> 1. Add useNewProducer in the MirrorMaker.
> 2. Remove MirrorMaker's dependency on MigrationTool's producer thread and data channel.
> 3. Unify Old/New Producer in a new class for MirrorMaker, ProducerPerformance and ConsoleProducer
>
> Will remove kafka.tools.newproducer.MirrorMaker upon final commit.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/producer/TransitProducer.scala PRE-CREATION
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432: address Jun's comments
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42185
-----------------------------------------------------------
core/src/main/scala/kafka/producer/BaseProducer.scala
<https://reviews.apache.org/r/20997/#comment75926>
transit -> base
core/src/main/scala/kafka/producer/BaseProducer.scala
<https://reviews.apache.org/r/20997/#comment75927>
sync send -> send synchronously
core/src/main/scala/kafka/producer/BaseProducer.scala
<https://reviews.apache.org/r/20997/#comment75928>
The comment says block on a full buffer, but the config turns it off.
core/src/main/scala/kafka/tools/MirrorMaker.scala
<https://reviews.apache.org/r/20997/#comment75932>
Now, there is a discrepancy in the names. MirrorMakerThread is too general and it is being referred to as consumerThread elsewhere. Should we just rename MirrorMakerThread to ConsumerThread?
core/src/main/scala/kafka/tools/MirrorMaker.scala
<https://reviews.apache.org/r/20997/#comment75930>
i->producerIndex
- Neha Narkhede
On May 5, 2014, 5:31 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 5:31 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Jun's comments
>
>
> Add new TransitProducer
>
>
> KAFKA-1432.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
> system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432: address Neha's comments
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42376
-----------------------------------------------------------
system_test/utils/kafka_system_test_utils.py
<https://reviews.apache.org/r/20997/#comment76155>
Shouldn't we add the --new-producer option?
- Jun Rao
On May 5, 2014, 10:24 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 10:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Neha's comments round two
>
>
> Address Neha's comments
>
>
> Address Jun's comments
>
>
> Add new TransitProducer
>
>
> KAFKA-1432.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.scala a969a22c6b82ed5c9f053a50594b1c7a768efd0e
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
> system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432: address Neha's comments
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42221
-----------------------------------------------------------
Ship it!
Ship It!
- Neha Narkhede
On May 5, 2014, 10:24 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 10:24 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Neha's comments round two
>
>
> Address Neha's comments
>
>
> Address Jun's comments
>
>
> Add new TransitProducer
>
>
> KAFKA-1432.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.scala a969a22c6b82ed5c9f053a50594b1c7a768efd0e
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
> system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432: address Neha's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/
-----------------------------------------------------------
(Updated May 5, 2014, 10:24 p.m.)
Review request for kafka.
Bugs: KAFKA-1432
https://issues.apache.org/jira/browse/KAFKA-1432
Repository: kafka
Description (updated)
-------
Address Neha's comments round two
Address Neha's comments
Address Jun's comments
Add new TransitProducer
KAFKA-1432.v1
Diffs (updated)
-----
core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.scala a969a22c6b82ed5c9f053a50594b1c7a768efd0e
perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
Diff: https://reviews.apache.org/r/20997/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20997: Fix KAFKA-1432: address Neha's comments
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42212
-----------------------------------------------------------
core/src/main/scala/kafka/tools/MirrorMaker.scala
<https://reviews.apache.org/r/20997/#comment75975>
This needs to change as well right? Maybe something like mirrormaker-consumer
core/src/main/scala/kafka/tools/MirrorMaker.scala
<https://reviews.apache.org/r/20997/#comment75976>
For consistency, this should be mirrormaker-producer
- Neha Narkhede
On May 5, 2014, 9:33 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 9:33 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Neha's comments
>
>
> Address Jun's comments
>
>
> Add new TransitProducer
>
>
> KAFKA-1432.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.scala a969a22c6b82ed5c9f053a50594b1c7a768efd0e
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
> system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432: address Neha's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/
-----------------------------------------------------------
(Updated May 5, 2014, 9:33 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1432: address Neha's comments
Bugs: KAFKA-1432
https://issues.apache.org/jira/browse/KAFKA-1432
Repository: kafka
Description (updated)
-------
Address Neha's comments
Address Jun's comments
Add new TransitProducer
KAFKA-1432.v1
Diffs (updated)
-----
core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.scala a969a22c6b82ed5c9f053a50594b1c7a768efd0e
perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
Diff: https://reviews.apache.org/r/20997/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20997: Fix KAFKA-1432: address Jun's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/
-----------------------------------------------------------
(Updated May 5, 2014, 5:31 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Fix KAFKA-1432: address Jun's comments
Bugs: KAFKA-1432
https://issues.apache.org/jira/browse/KAFKA-1432
Repository: kafka
Description (updated)
-------
Address Jun's comments
Add new TransitProducer
KAFKA-1432.v1
Diffs (updated)
-----
core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
Diff: https://reviews.apache.org/r/20997/diff/
Testing
-------
Thanks,
Guozhang Wang
Re: Review Request 20997: Fix KAFKA-1432: address Jun's comments
Posted by Neha Narkhede <ne...@gmail.com>.
> On May 2, 2014, 5:51 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/producer/ConsoleProducer.scala, line 50
> > <https://reviews.apache.org/r/20997/diff/1/?file=573267#file573267line50>
> >
> > It will be cleaner to move this logic into a buildProducerConfig private API that builds either the new or old producer's configs based on some boolean value.
>
> Guozhang Wang wrote:
> The thing is that this buildProducerConfig would not be able to be shared between ConsoleProducer and ProducerPerformance since the former used its own ProducerConfig while the latter used its own ProducerPerfConfig. So we would have two buildProducerConfig functions in these two classes and just move the logic there. I think it would not help much for the cleaning.
makes sense.
> On May 2, 2014, 5:51 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/producer/TransitProducer.scala, lines 24-25
> > <https://reviews.apache.org/r/20997/diff/1/?file=573268#file573268line24>
> >
> > +1 on renaming this as per Jun's suggestion.
> >
> > If this is only for the tools, then let's move it to the tools package.
>
> Guozhang Wang wrote:
> ConsoleProducer is in kafka.producer
> ProducerPerformance is in kafka.perf
> MirrorMaker is in kafka.tools
>
> So I decided to put the BaseProducer in kafka.producer. Let me know if you have another preference.
ConsoleProducer is a tool and should be moved to kafka.tools
ProducerPerformance is also supposed to be in kafka.tools.
It seems to me that it's better to do these moves and also move BaseProducer to kafka.tools.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42028
-----------------------------------------------------------
On May 5, 2014, 5:31 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 5:31 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Jun's comments
>
>
> Add new TransitProducer
>
>
> KAFKA-1432.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
> system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432
Posted by Guozhang Wang <gu...@linkedin.com>.
> On May 2, 2014, 5:51 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/producer/TransitProducer.scala, lines 24-25
> > <https://reviews.apache.org/r/20997/diff/1/?file=573268#file573268line24>
> >
> > +1 on renaming this as per Jun's suggestion.
> >
> > If this is only for the tools, then let's move it to the tools package.
ConsoleProducer is in kafka.producer
ProducerPerformance is in kafka.perf
MirrorMaker is in kafka.tools
So I decided to put the BaseProducer in kafka.producer. Let me know if you have another preference.
> On May 2, 2014, 5:51 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/producer/ConsoleProducer.scala, line 50
> > <https://reviews.apache.org/r/20997/diff/1/?file=573267#file573267line50>
> >
> > It will be cleaner to move this logic into a buildProducerConfig private API that builds either the new or old producer's configs based on some boolean value.
The thing is that this buildProducerConfig would not be able to be shared between ConsoleProducer and ProducerPerformance since the former used its own ProducerConfig while the latter used its own ProducerPerfConfig. So we would have two buildProducerConfig functions in these two classes and just move the logic there. I think it would not help much for the cleaning.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42028
-----------------------------------------------------------
On May 1, 2014, 11:46 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 11:46 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> 1. Add useNewProducer in the MirrorMaker.
> 2. Remove MirrorMaker's dependency on MigrationTool's producer thread and data channel.
> 3. Unify Old/New Producer in a new class for MirrorMaker, ProducerPerformance and ConsoleProducer
>
> Will remove kafka.tools.newproducer.MirrorMaker upon final commit.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/producer/TransitProducer.scala PRE-CREATION
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432: address Jun's comments
Posted by Guozhang Wang <gu...@linkedin.com>.
> On May 2, 2014, 5:51 p.m., Neha Narkhede wrote:
> > core/src/main/scala/kafka/producer/TransitProducer.scala, lines 24-25
> > <https://reviews.apache.org/r/20997/diff/1/?file=573268#file573268line24>
> >
> > +1 on renaming this as per Jun's suggestion.
> >
> > If this is only for the tools, then let's move it to the tools package.
>
> Guozhang Wang wrote:
> ConsoleProducer is in kafka.producer
> ProducerPerformance is in kafka.perf
> MirrorMaker is in kafka.tools
>
> So I decided to put the BaseProducer in kafka.producer. Let me know if you have another preference.
>
> Neha Narkhede wrote:
> ConsoleProducer is a tool and should be moved to kafka.tools
> ProducerPerformance is also supposed to be in kafka.tools.
>
> It seems to me that it's better to do these moves and also move BaseProducer to kafka.tools.
Agreed, but I would suggest we do this migration in another JIRA, where we can:
1. Move Consumer/ProducerPerformance and SimpleConsumerPerformance to tools and remove the perf sub-project.
2. Move ConsoleConsumer from kafka.consumer to kafka.tools.
3. Move other consumer related tools from kafka.consumer to kafka.tools.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42028
-----------------------------------------------------------
On May 5, 2014, 5:31 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 5, 2014, 5:31 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Address Jun's comments
>
>
> Add new TransitProducer
>
>
> KAFKA-1432.v1
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/BaseProducer.scala PRE-CREATION
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
> system_test/utils/kafka_system_test_utils.py 423b512983c77b41b819912fe67b375a78faf644
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42028
-----------------------------------------------------------
The patch failed to apply on trunk. Will complete my review once you update the diff.
core/src/main/scala/kafka/producer/ConsoleProducer.scala
<https://reviews.apache.org/r/20997/#comment75774>
It will be cleaner to move this logic into a buildProducerConfig private API that builds either the new or old producer's configs based on some boolean value.
core/src/main/scala/kafka/producer/TransitProducer.scala
<https://reviews.apache.org/r/20997/#comment75775>
+1 on renaming this as per Jun's suggestion.
If this is only for the tools, then let's move it to the tools package.
- Neha Narkhede
On May 1, 2014, 11:46 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 11:46 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> 1. Add useNewProducer in the MirrorMaker.
> 2. Remove MirrorMaker's dependency on MigrationTool's producer thread and data channel.
> 3. Unify Old/New Producer in a new class for MirrorMaker, ProducerPerformance and ConsoleProducer
>
> Will remove kafka.tools.newproducer.MirrorMaker upon final commit.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/producer/TransitProducer.scala PRE-CREATION
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/#review42037
-----------------------------------------------------------
Also, if we want to remove kafka.tools.newproducer.MirrorMaker, could we patch system tests too? It seems that we just need to patch the following line.
system_test//utils/kafka_system_test_utils.py: kafkaHome + "/bin/kafka-run-class.sh kafka.tools.newproducer.MirrorMaker",
- Jun Rao
On May 1, 2014, 11:46 p.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20997/
> -----------------------------------------------------------
>
> (Updated May 1, 2014, 11:46 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1432
> https://issues.apache.org/jira/browse/KAFKA-1432
>
>
> Repository: kafka
>
>
> Description
> -------
>
> 1. Add useNewProducer in the MirrorMaker.
> 2. Remove MirrorMaker's dependency on MigrationTool's producer thread and data channel.
> 3. Unify Old/New Producer in a new class for MirrorMaker, ProducerPerformance and ConsoleProducer
>
> Will remove kafka.tools.newproducer.MirrorMaker upon final commit.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
> core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
> core/src/main/scala/kafka/producer/TransitProducer.scala PRE-CREATION
> core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
> perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
>
> Diff: https://reviews.apache.org/r/20997/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guozhang Wang
>
>
Re: Review Request 20997: Fix KAFKA-1432
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20997/
-----------------------------------------------------------
(Updated May 1, 2014, 11:46 p.m.)
Review request for kafka.
Bugs: KAFKA-1432
https://issues.apache.org/jira/browse/KAFKA-1432
Repository: kafka
Description (updated)
-------
1. Add useNewProducer in the MirrorMaker.
2. Remove MirrorMaker's dependency on MigrationTool's producer thread and data channel.
3. Unify Old/New Producer in a new class for MirrorMaker, ProducerPerformance and ConsoleProducer
Will remove kafka.tools.newproducer.MirrorMaker upon final commit.
Diffs
-----
core/src/main/scala/kafka/producer/ByteArrayPartitioner.scala 988e4374d8c9b30c9333369741c30c75f0c44f2c
core/src/main/scala/kafka/producer/ConsoleProducer.scala b19ab49c5fd57423e142f2f8afc5a77e653fd6ed
core/src/main/scala/kafka/producer/TransitProducer.scala PRE-CREATION
core/src/main/scala/kafka/tools/MirrorMaker.scala e4d1a86f673f7df7fe71ce44e3550fcde8d01bba
perf/src/main/scala/kafka/perf/ProducerPerformance.scala 1490bdb3d52d5c901d7a464222284935bda2f7ca
Diff: https://reviews.apache.org/r/20997/diff/
Testing
-------
Thanks,
Guozhang Wang