Re: Review Request 31606: Patch for KAFKA-1416

[Guozhang Wang <wa...@gmail.com>]

> On March 25, 2015, 9:51 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/utils/TestUtils.scala, lines 761-773
> > <https://reviews.apache.org/r/31606/diff/2/?file=881937#file881937line761>
> >
> >     Compression code is no longer used anymore, which seems not correct?
> 
> Flutra Osmani wrote:
>     The compression.codec is now set on sendMessages()

Isn't this function sendMessages()? Now when we call createProducer() we will not pass in the props anymore and hence it will always use default values, is that OK?


> On March 25, 2015, 9:51 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala, lines 116-134
> > <https://reviews.apache.org/r/31606/diff/2/?file=881935#file881935line116>
> >
> >     Can we get rid of this function since it is only called once, and hence we can just put the logic there?
> 
> Flutra Osmani wrote:
>     Same as above:
>     In kafka.javaapi.consumer.ZookeeperConsumerConnectorTest.scala is the intention to test the Java API integration, or the actual functionality of getMessages in this case?

I was not thinking about replacing it with getMessages, but just put this function logic to the place where it is triggered as it is only called once. I think it is OK to just keep it as is now.


> On March 25, 2015, 9:51 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala, lines 85-106
> > <https://reviews.apache.org/r/31606/diff/2/?file=881935#file881935line85>
> >
> >     Can we use TestUtils.sendMessages to replace this function?
> 
> Flutra Osmani wrote:
>     My understanding was that we wanted to test kafka.javaapi.producer.Producer here (not necessarily kafka.producer.Producer).
>     
>     In kafka.javaapi.consumer.ZookeeperConsumerConnectorTest.scala is the intention to test the Java API integration, or the actual functionality of sendMessages?

Hmm that is a good point. I think we can just keep the java producer here, but with a bit refactoring: for example we can get rid of the compression type as it is not used anywhere.


- Guozhang


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


On March 26, 2015, 7:21 a.m., Flutra Osmani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31606/
> -----------------------------------------------------------
> 
> (Updated March 26, 2015, 7:21 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1416
>     https://issues.apache.org/jira/browse/KAFKA-1416
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> Unified get and send messages in TestUtils.scala and its users
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 155fd046c9036b5b8b107bd21a8c6492c14644ea 
>   core/src/test/scala/unit/kafka/integration/FetcherTest.scala 3093e459935ecf8e5b34fca34a422674562a7034 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala 8342cae564ebc39fe74a512343a4523072ca205a 
>   core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala 3d0fc9deda2d3a39f2618a5be3edd98cd935ffbb 
>   core/src/test/scala/unit/kafka/metrics/MetricsTest.scala 0f58ad8e698e3c0ec76c510bd5f76912a992209c 
>   core/src/test/scala/unit/kafka/server/LogRecoveryTest.scala 92d6b2c672f74cdd526f2e98da8f7fb3696a88e3 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 1682a77362123449de6bb1d54a55887409990a24 
> 
> Diff: https://reviews.apache.org/r/31606/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Flutra Osmani
> 
>