You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Neha Narkhede <ne...@gmail.com> on 2014/02/03 02:02:31 UTC
Review Request 17649: Patch for KAFKA-1237
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1237
https://issues.apache.org/jira/browse/KAFKA-1237
Repository: kafka
Description
-------
Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
Diffs
-----
bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
Diff: https://reviews.apache.org/r/17649/diff/
Testing
-------
Thanks,
Neha Narkhede
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 3, 2014, 9:48 p.m., Guozhang Wang wrote:
> > Conditional on the producer clean shutdown :)
>
> Guozhang Wang wrote:
> I'm wondering if you are still able to build/release with the changes in Build.scala? In my case it will cause ambiguous reference error.
yes, I'm able to build.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33511
-----------------------------------------------------------
On Feb. 4, 2014, 1:26 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 1:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Feb. 3, 2014, 9:48 p.m., Guozhang Wang wrote:
> > Conditional on the producer clean shutdown :)
>
> Guozhang Wang wrote:
> I'm wondering if you are still able to build/release with the changes in Build.scala? In my case it will cause ambiguous reference error.
>
> Neha Narkhede wrote:
> yes, I'm able to build.
Could you try ./sbt release-tar?
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33511
-----------------------------------------------------------
On Feb. 4, 2014, 1:26 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 1:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Feb. 3, 2014, 9:48 p.m., Guozhang Wang wrote:
> > Conditional on the producer clean shutdown :)
I'm wondering if you are still able to build/release with the changes in Build.scala? In my case it will cause ambiguous reference error.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33511
-----------------------------------------------------------
On Feb. 3, 2014, 1:02 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 1:02 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33511
-----------------------------------------------------------
Ship it!
Conditional on the producer clean shutdown :)
- Guozhang Wang
On Feb. 3, 2014, 1:02 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 1:02 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 3, 2014, 7:55 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 81
> > <https://reviews.apache.org/r/17649/diff/1/?file=462835#file462835line81>
> >
> > Can you fix the whitespace issues?
I'm actually not sure what's introducing the whitespaces in eclipse and how to get rid of those
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33480
-----------------------------------------------------------
On Feb. 4, 2014, 1:26 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 1:26 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 3, 2014, 7:55 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 118
> > <https://reviews.apache.org/r/17649/diff/1/?file=462835#file462835line118>
> >
> > Can we try just one producer thread? Also, if you do go with multiple producers then I think you will need to extract the key and if a key is present then send it to the a designated producer thread to ensure ordering within keys. (This is currently done in the existing mirror maker).
Included multiple producer mainly to allow scaling producer throughput within one mirror maker instance, since I'm not currently sure if the producer throughput is high enough. We can always start by using just one producer. You have a good point about distributing the keyed data, will include that in the updated diff
> On Feb. 3, 2014, 7:55 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 125
> > <https://reviews.apache.org/r/17649/diff/1/?file=462835#file462835line125>
> >
> > Producers are not shut down cleanly.
This was in my local diff, forgot to upload it. Will include it in the next diff.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33480
-----------------------------------------------------------
On Feb. 3, 2014, 1:02 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 1:02 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33480
-----------------------------------------------------------
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment62927>
Can you fix the whitespace issues?
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment62932>
Can we try just one producer thread? Also, if you do go with multiple producers then I think you will need to extract the key and if a key is present then send it to the a designated producer thread to ensure ordering within keys. (This is currently done in the existing mirror maker).
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment62934>
Producers are not shut down cleanly.
- Joel Koshy
On Feb. 3, 2014, 1:02 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2014, 1:02 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33606
-----------------------------------------------------------
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment63141>
I'm still trying to figure out the best way to get rid of these whitespaces.
- Neha Narkhede
On Feb. 4, 2014, 5:10 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 5:10 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33830
-----------------------------------------------------------
Ship it!
Ship It!
- Guozhang Wang
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 7, 2014, 4:53 a.m., Jay Kreps wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 25
> > <https://reviews.apache.org/r/17649/diff/7/?file=469305#file469305line25>
> >
> > I don't think we should have a list of producers. We should just have a single producer shared across all mirror threads so that batching is more effective.
Yes, thought of that earlier, though not sure how much throughput the producer can currently sustain. To allow us to play with a new settings on the number of producers, added one producer per stream. We can come back and reduce it to a single producer if the throughput is high enough.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33904
-----------------------------------------------------------
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33904
-----------------------------------------------------------
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment63623>
I don't think we should have a list of producers. We should just have a single producer shared across all mirror threads so that batching is more effective.
- Jay Kreps
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 7, 2014, 4:50 a.m., Jay Kreps wrote:
> > I want to second Joel's comment on Java. I think/thought the plan was to keep the server in scala at least for the time being. I would be in favor of moving the server to Java too, but I think if we want to do that we might need to hole up and just rewrite it all. If we have some of each it will be pretty painful I think.
Makes sense. With that, I will discard this reviewboard and wait until the gradle patch is in. Currently my entire IDE is broken while trying make the server depend on the client and it is annoying to proceed like this.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33903
-----------------------------------------------------------
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33903
-----------------------------------------------------------
I want to second Joel's comment on Java. I think/thought the plan was to keep the server in scala at least for the time being. I would be in favor of moving the server to Java too, but I think if we want to do that we might need to hole up and just rewrite it all. If we have some of each it will be pretty painful I think.
- Jay Kreps
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 6, 2014, 9:43 p.m., Jay Kreps wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 41
> > <https://reviews.apache.org/r/17649/diff/7/?file=469305#file469305line41>
> >
> > I don't think this works as the key is a byte[] so the hashCode is based on the object instance rather than the key contents.
That's true, I seemed to have overlooked that.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33845
-----------------------------------------------------------
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33845
-----------------------------------------------------------
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment63462>
I don't think this works as the key is a byte[] so the hashCode is based on the object instance rather than the key contents.
- Jay Kreps
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
> On Feb. 6, 2014, 10:36 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 41
> > <https://reviews.apache.org/r/17649/diff/7/?file=469305#file469305line41>
> >
> > That's right.
> >
> > The existing mirror maker uses Utils.abs(java.util.Arrays.hashCode...) We should just stick with that.
Yup. Jay already pointed it out in his review earlier.
> On Feb. 6, 2014, 10:36 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 89
> > <https://reviews.apache.org/r/17649/diff/7/?file=469305#file469305line89>
> >
> > It's a bit odd to have this written in java and be under
> > core/src/main/scala.
> >
> > Also, if this is not a temporary implementation then can you use an option
> > parser as opposed to directly grabbing the CLI args?
> >
> > Also, I don't see any of the logging from before which we would want even if it is a temporary tool under test.
> >
I think most of these will have to be addressed in a new patch and rb as it seems like it is better to keep it in Scala.
> On Feb. 6, 2014, 10:36 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java, line 71
> > <https://reviews.apache.org/r/17649/diff/7/?file=469305#file469305line71>
> >
> > If the producer is configured to throw QFE or if it throws any other exception, then this would effectively stop this mirror thread.
Producer cannot throw QFE in blocking mode which is hard coded in the mirror maker and if it throws some other unexpected error, that is a bug in the mirror maker and it should exist rather than continue.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33851
-----------------------------------------------------------
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/#review33851
-----------------------------------------------------------
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment63469>
That's right.
The existing mirror maker uses Utils.abs(java.util.Arrays.hashCode...) We should just stick with that.
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment63472>
If the producer is configured to throw QFE or if it throws any other exception, then this would effectively stop this mirror thread.
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment63465>
It's a bit odd to have this written in java and be under
core/src/main/scala.
Also, if this is not a temporary implementation then can you use an option
parser as opposed to directly grabbing the CLI args?
Also, I don't see any of the logging from before which we would want even if it is a temporary tool under test.
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java
<https://reviews.apache.org/r/17649/#comment63474>
If the mirror thread exits abnormally then it would be better to close the producers cleanly as well over here. (This is one reason the existing mirror maker uses a cleanShutdown method that we call from the shutdown hook as well as here.)
- Joel Koshy
On Feb. 4, 2014, 9:12 p.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17649/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2014, 9:12 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1137 and KAFKA-1237
> https://issues.apache.org/jira/browse/KAFKA-1137
> https://issues.apache.org/jira/browse/KAFKA-1237
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Removed the tabs
>
>
> Rebased to new producer API changes
>
>
> removing whitespaces
>
>
> removing whitespaces
>
>
> Joel's review suggestions
>
>
> Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
> core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
> project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
>
> Diff: https://reviews.apache.org/r/17649/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neha Narkhede
>
>
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 9:12 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Patch for KAFKA-1237
Bugs: KAFKA-1137 and KAFKA-1237
https://issues.apache.org/jira/browse/KAFKA-1137
https://issues.apache.org/jira/browse/KAFKA-1237
Repository: kafka
Description
-------
Removed the tabs
Rebased to new producer API changes
removing whitespaces
removing whitespaces
Joel's review suggestions
Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
Diffs (updated)
-----
bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
Diff: https://reviews.apache.org/r/17649/diff/
Testing
-------
Thanks,
Neha Narkhede
Re: Review Request 17649: Patch for KAFKA-1137
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 9:04 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Patch for KAFKA-1137
Bugs: KAFKA-1137 and KAFKA-1237
https://issues.apache.org/jira/browse/KAFKA-1137
https://issues.apache.org/jira/browse/KAFKA-1237
Repository: kafka
Description (updated)
-------
Removed the tabs
Rebased to new producer API changes
removing whitespaces
removing whitespaces
Joel's review suggestions
Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
Diffs (updated)
-----
bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
Diff: https://reviews.apache.org/r/17649/diff/
Testing
-------
Thanks,
Neha Narkhede
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 5:10 p.m.)
Review request for kafka.
Bugs: KAFKA-1237
https://issues.apache.org/jira/browse/KAFKA-1237
Repository: kafka
Description (updated)
-------
Rebased to new producer API changes
removing whitespaces
removing whitespaces
Joel's review suggestions
Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
Diffs (updated)
-----
bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
Diff: https://reviews.apache.org/r/17649/diff/
Testing
-------
Thanks,
Neha Narkhede
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 1:26 a.m.)
Review request for kafka.
Bugs: KAFKA-1237
https://issues.apache.org/jira/browse/KAFKA-1237
Repository: kafka
Description (updated)
-------
removing whitespaces
removing whitespaces
Joel's review suggestions
Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
Diffs (updated)
-----
bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
Diff: https://reviews.apache.org/r/17649/diff/
Testing
-------
Thanks,
Neha Narkhede
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 1:25 a.m.)
Review request for kafka.
Bugs: KAFKA-1237
https://issues.apache.org/jira/browse/KAFKA-1237
Repository: kafka
Description (updated)
-------
removing whitespaces
Joel's review suggestions
Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
Diffs (updated)
-----
bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
Diff: https://reviews.apache.org/r/17649/diff/
Testing
-------
Thanks,
Neha Narkhede
Re: Review Request 17649: Patch for KAFKA-1237
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17649/
-----------------------------------------------------------
(Updated Feb. 4, 2014, 1:22 a.m.)
Review request for kafka.
Bugs: KAFKA-1237
https://issues.apache.org/jira/browse/KAFKA-1237
Repository: kafka
Description (updated)
-------
Joel's review suggestions
Mirror maker using new producer. Does not work for auto created topics (KAFKA-1238)
Diffs (updated)
-----
bin/kafka-run-class.sh 416ecadf27333d1524518e8229541d161449e282
core/src/main/scala/kafka/tools/newproducer/MirrorMaker.java PRE-CREATION
project/Build.scala ddcfc4176e68933377590e095c07283083249f4a
Diff: https://reviews.apache.org/r/17649/diff/
Testing
-------
Thanks,
Neha Narkhede