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/04 22:04:31 UTC

Re: Review Request 17649: Patch for KAFKA-1137

-----------------------------------------------------------
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 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