Re: Review Request 35615: Patch for KAFKA-1782

2015-07-16 Thread Guozhang Wang 

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


I can consistently get 8 unit test failues with your patch, all of them on 
"kafka.server.ServerShutdownTest.verifyNonDaemonThreadsStatus":

java.lang.AssertionError: expected:<0> but was:<22>
at org.junit.Assert.fail(Assert.java:92)
at org.junit.Assert.failNotEquals(Assert.java:689)
at org.junit.Assert.assertEquals(Assert.java:127)
at org.junit.Assert.assertEquals(Assert.java:514)
at org.junit.Assert.assertEquals(Assert.java:498)
at 
kafka.server.ServerShutdownTest.verifyNonDaemonThreadsStatus(ServerShutdownTest.scala:157)
at 
kafka.server.ServerShutdownTest.testCleanShutdown(ServerShutdownTest.scala:104)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
at 
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
at 
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:31)
at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
at 
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:44)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:180)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:41)
at org.junit.runners.ParentRunner$1.evaluate(ParentRunner.java:173)
at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
at 
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:31)
at org.junit.runners.ParentRunner.run(ParentRunner.java:220)
at 
org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecuter.runTestClass(JUnitTestClassExecuter.java:86)
at 
org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecuter.execute(JUnitTestClassExecuter.java:49)
at 
org.gradle.api.internal.tasks.testing.junit.JUnitTestClassProcessor.processTestClass(JUnitTestClassProcessor.java:64)
at 
org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:50)
at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at 
org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
at 
org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
at 
org.gradle.messaging.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:32)
at 
org.gradle.messaging.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:93)
at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
at 
org.gradle.api.internal.tasks.testing.worker.TestWorker.processTestClass(TestWorker.java:106)
at sun.reflect.GeneratedMethodAccessor10.invoke(Unknown Source)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at 
org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
at 
org.gradle.messaging.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
at 
org.gradle.messaging.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:360)
at 
org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:54)
at 
org.gradle.internal.concurrent.StoppableExecutorImpl$1.run(StoppableExecutorImpl.java:40)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)

- Guozhang Wang


On July 16, 2015, 6:56 p.m., Alexander Pakulov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35615/
> -

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-16 Thread Guozhang Wang 


> On July 16, 2015, 4:54 p.m., Guozhang Wang wrote:
> > Could you rebase? Seems some previous commits get reflected in your latest 
> > patch.
> 
> Alexander Pakulov wrote:
> Done.
> 
> Also I have an update for BeforeAndAfter\BeforeAndAfterEach traits. These 
> traits requires any of Suite traits to defined for the class also. E.g.
> 
> BeforeAndAfter - non-stackable solution
> ```java
> class SchedulerTest extends Suite with BeforeAndAfter {
> 
>   val scheduler = new KafkaScheduler(1)
>   var mockTime: MockTime = null
>   var counter1: AtomicInteger = null
>   var counter2: AtomicInteger = null
>   
>   before {
> mockTime = new MockTime
> counter1 = new AtomicInteger(0)
> counter2 = new AtomicInteger(0)
> scheduler.startup()
>   }
>   
>   after {
> scheduler.shutdown()
>   }
> 
>   def testMockSchedulerNonPeriodicTask() {}
> ```
> 
> BeforeAndAfterEach
> ```java
> class SchedulerTest extends Suite with BeforeAndAfterEach {
> 
>   val scheduler = new KafkaScheduler(1)
>   var mockTime: MockTime = null
>   var counter1: AtomicInteger = null
>   var counter2: AtomicInteger = null
>   
>   override def beforeEach {
> super.beforeEach()
> mockTime = new MockTime
> counter1 = new AtomicInteger(0)
> counter2 = new AtomicInteger(0)
> scheduler.startup()
>   }
>   
>   override def afterEach {
> scheduler.shutdown()
> super.afterEach()
>   }
> 
>   def testMockSchedulerNonPeriodicTask() {}
> ```
> 
> Pure JUnit
> ```java
> class SchedulerTestJUnit {
> 
>   val scheduler = new KafkaScheduler(1)
>   val mockTime = new MockTime
>   val counter1 = new AtomicInteger(0)
>   val counter2 = new AtomicInteger(0)
> 
>   @Before
>   def setUp {
> scheduler.startup()
>   }
>   
>   @After
>   def tearDown {
> scheduler.shutdown()
>   }
> 
>   @Test
>   def testMockSchedulerNonPeriodicTask() {}
> ```
> 
> Since majority of the tests were written with JUnit flavor, I'd better 
> stay with Pure JUnit implementation for now. Once one we decide to be 
> ScalaTest heavy - there will be another refactoring. 
> 
> We could create a new ticket for this migration, what do you think?

Makes sense, thanks for the update!


- Guozhang


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


On July 16, 2015, 6:56 p.m., Alexander Pakulov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35615/
> ---
> 
> (Updated July 16, 2015, 6:56 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1782
> https://issues.apache.org/jira/browse/KAFKA-1782
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1782; Junit3 Misusage
> 
> 
> Diffs
> -
> 
>   core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
> b0750faa43dfe21a9226335020b4b5dac6cf65bf 
>   core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
> 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca 
>   core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
> afcc349342d44f42e19ca730dbd074c338638a64 
>   core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
> ce70a0a449883723a9b59ea48da34ba30b3f6daf 
>   core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
> 83de81cb3f79a6966dd5ef462733d8a22cd6d467 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
> ee94011894b46864614b97bbd2a98375a7d3f20b 
>   core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
> 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
>   core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
> 4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
>   core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
> df5c6ba20f01e497ce896af790cbab40369f1776 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala 
> 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 
>   core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
> 1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
> fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
>   core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
> dcd69881445c29765f66a7d21d2d18437f4df428 
>   core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
> 255442526d94157b7a0b5a92e1d6a900aacb7536 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
> 5717165f2344823fabe8f7cfafae4bb8af2d949a 
>   core/

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-16 Thread Alexander Pakulov 


> On July 16, 2015, 4:54 p.m., Guozhang Wang wrote:
> > Could you rebase? Seems some previous commits get reflected in your latest 
> > patch.

Done.

Also I have an update for BeforeAndAfter\BeforeAndAfterEach traits. These 
traits requires any of Suite traits to defined for the class also. E.g.

BeforeAndAfter - non-stackable solution
```java
class SchedulerTest extends Suite with BeforeAndAfter {

  val scheduler = new KafkaScheduler(1)
  var mockTime: MockTime = null
  var counter1: AtomicInteger = null
  var counter2: AtomicInteger = null
  
  before {
mockTime = new MockTime
counter1 = new AtomicInteger(0)
counter2 = new AtomicInteger(0)
scheduler.startup()
  }
  
  after {
scheduler.shutdown()
  }

  def testMockSchedulerNonPeriodicTask() {}
```

BeforeAndAfterEach
```java
class SchedulerTest extends Suite with BeforeAndAfterEach {

  val scheduler = new KafkaScheduler(1)
  var mockTime: MockTime = null
  var counter1: AtomicInteger = null
  var counter2: AtomicInteger = null
  
  override def beforeEach {
super.beforeEach()
mockTime = new MockTime
counter1 = new AtomicInteger(0)
counter2 = new AtomicInteger(0)
scheduler.startup()
  }
  
  override def afterEach {
scheduler.shutdown()
super.afterEach()
  }

  def testMockSchedulerNonPeriodicTask() {}
```

Pure JUnit
```java
class SchedulerTestJUnit {

  val scheduler = new KafkaScheduler(1)
  val mockTime = new MockTime
  val counter1 = new AtomicInteger(0)
  val counter2 = new AtomicInteger(0)

  @Before
  def setUp {
scheduler.startup()
  }
  
  @After
  def tearDown {
scheduler.shutdown()
  }

  @Test
  def testMockSchedulerNonPeriodicTask() {}
```

Since majority of the tests were written with JUnit flavor, I'd better stay 
with Pure JUnit implementation for now. Once one we decide to be ScalaTest 
heavy - there will be another refactoring. 

We could create a new ticket for this migration, what do you think?


- Alexander


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


On July 16, 2015, 6:56 p.m., Alexander Pakulov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35615/
> ---
> 
> (Updated July 16, 2015, 6:56 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1782
> https://issues.apache.org/jira/browse/KAFKA-1782
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1782; Junit3 Misusage
> 
> 
> Diffs
> -
> 
>   core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
> b0750faa43dfe21a9226335020b4b5dac6cf65bf 
>   core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
> 3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca 
>   core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
> afcc349342d44f42e19ca730dbd074c338638a64 
>   core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
> ce70a0a449883723a9b59ea48da34ba30b3f6daf 
>   core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
> 83de81cb3f79a6966dd5ef462733d8a22cd6d467 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
> ee94011894b46864614b97bbd2a98375a7d3f20b 
>   core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
> 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
>   core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
> 4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
>   core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
> df5c6ba20f01e497ce896af790cbab40369f1776 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala 
> 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 
>   core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
> 1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
> fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
>   core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
> dcd69881445c29765f66a7d21d2d18437f4df428 
>   core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
> 255442526d94157b7a0b5a92e1d6a900aacb7536 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
> 5717165f2344823fabe8f7cfafae4bb8af2d949a 
>   core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala 
> abe511fc1458bfb5a93c152aed81a827cc24ce68 
>   core/src/test/scala/unit/kafka/common/ConfigTest.scala 
> 0aca9385bff093b4cb52e42291c9c1d58d9600de 
>   core/src/test/scala/unit/kafka/common/TopicTest.scala 
> 79532c89c41572ba953c4dc3319a05354927e961 
>   core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala 
> db5302ff02851f9f1a59419b1e071286bff0e142 
>   core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala 
> adf08010597b7c6ed72eddf93962497c3209e

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-16 Thread Alexander Pakulov 

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

(Updated July 16, 2015, 6:56 p.m.)


Review request for kafka.


Bugs: KAFKA-1782
https://issues.apache.org/jira/browse/KAFKA-1782


Repository: kafka


Description
---

KAFKA-1782; Junit3 Misusage


Diffs (updated)
-

  core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
b0750faa43dfe21a9226335020b4b5dac6cf65bf 
  core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca 
  core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
afcc349342d44f42e19ca730dbd074c338638a64 
  core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
ce70a0a449883723a9b59ea48da34ba30b3f6daf 
  core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
83de81cb3f79a6966dd5ef462733d8a22cd6d467 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
ee94011894b46864614b97bbd2a98375a7d3f20b 
  core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
  core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
  core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
df5c6ba20f01e497ce896af790cbab40369f1776 
  core/src/test/scala/unit/kafka/admin/AdminTest.scala 
252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 
  core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
  core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
dcd69881445c29765f66a7d21d2d18437f4df428 
  core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
255442526d94157b7a0b5a92e1d6a900aacb7536 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
5717165f2344823fabe8f7cfafae4bb8af2d949a 
  core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala 
abe511fc1458bfb5a93c152aed81a827cc24ce68 
  core/src/test/scala/unit/kafka/common/ConfigTest.scala 
0aca9385bff093b4cb52e42291c9c1d58d9600de 
  core/src/test/scala/unit/kafka/common/TopicTest.scala 
79532c89c41572ba953c4dc3319a05354927e961 
  core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala 
db5302ff02851f9f1a59419b1e071286bff0e142 
  core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala 
adf08010597b7c6ed72eddf93962497c3209e10f 
  core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala 
4b326d090c9486c812afb2603e94d77a2459d04c 
  core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 
359b0f5d14f82d14ee423cde271bb35b7034766d 
  
core/src/test/scala/unit/kafka/coordinator/ConsumerCoordinatorResponseTest.scala
 87a5330e716b9cedc9544229e29f42be7150c8fb 
  core/src/test/scala/unit/kafka/coordinator/ConsumerGroupMetadataTest.scala 
b69c993a3fa49f7f01dd28e13ce465c2a89eeba5 
  core/src/test/scala/unit/kafka/coordinator/CoordinatorMetadataTest.scala 
2cbf6e251adbfd3fb174d08138a13215c1914566 
  core/src/test/scala/unit/kafka/coordinator/PartitionAssignorTest.scala 
887cee5a582b5737ba838920399bb9b24bf22382 
  core/src/test/scala/unit/kafka/integration/AutoOffsetResetTest.scala 
139dc9a104c024e35fd9bc5ac9333e6bd208b571 
  core/src/test/scala/unit/kafka/integration/FetcherTest.scala 
facebd8f81c67f26f41a96bce343227bc9b55893 
  core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 
87c631573aa1e0275d6618c4ac3b33e76fa6abd3 
  core/src/test/scala/unit/kafka/integration/MinIsrConfigTest.scala 
a2c97134d85c637256440b5eb42f594d50f0cfe4 
  core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala 
6a758a7db71bdc4794b1e5f264f8ca6d410ff2ba 
  core/src/test/scala/unit/kafka/integration/ProducerConsumerTestHarness.scala 
4614a922e739098dbb0ff560d831e26045e32023 
  core/src/test/scala/unit/kafka/integration/RollingBounceTest.scala 
12d0733f5edf436315f884bc193da533d9d4a4ee 
  core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 
5b6c9d60d29436036f8287da6bad332c1a3a6ec9 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala 
e4bf2df48dd59a251b646b7f96d63ec4b924fc0b 
  
core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala
 74c761dec7afc98667032bdec8359a9aa7c2ecc2 
  core/src/test/scala/unit/kafka/javaapi/message/BaseMessageSetTestCases.scala 
726399e3c7a4157223b5037ff2a03da51236e876 
  core/src/test/scala/unit/kafka/javaapi/message/ByteBufferMessageSetTest.scala 
383fcef02994fde07e669651e522b9e5ee239dd8 
  core/src/test/scala/unit/kafka/log/CleanerTest.scala 
0e2a6a1e8e6d0bc6bed90cdc5bd3cb8e490ed364 
  core/src/test/scala/unit/kafka/log/FileMessageSetTest.scala 
02cf66882f4065f3ab8449650b0bd1b0e7525d38 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-16 Thread Alexander Pakulov 

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

(Updated July 16, 2015, 6:50 p.m.)


Review request for kafka.


Bugs: KAFKA-1782
https://issues.apache.org/jira/browse/KAFKA-1782


Repository: kafka


Description (updated)
---

KAFKA-1782; Junit3 Misusage


Diffs (updated)
-

  core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
b0750faa43dfe21a9226335020b4b5dac6cf65bf 
  core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
3eb5f95731a3f06f662b334ab2b3d0ad7fa9e1ca 
  core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
afcc349342d44f42e19ca730dbd074c338638a64 
  core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
ce70a0a449883723a9b59ea48da34ba30b3f6daf 
  core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
83de81cb3f79a6966dd5ef462733d8a22cd6d467 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
ee94011894b46864614b97bbd2a98375a7d3f20b 
  core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
  core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
  core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
df5c6ba20f01e497ce896af790cbab40369f1776 
  core/src/test/scala/unit/kafka/admin/AdminTest.scala 
252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 
  core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
  core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
dcd69881445c29765f66a7d21d2d18437f4df428 
  core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
255442526d94157b7a0b5a92e1d6a900aacb7536 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
5717165f2344823fabe8f7cfafae4bb8af2d949a 
  core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala 
abe511fc1458bfb5a93c152aed81a827cc24ce68 
  core/src/test/scala/unit/kafka/common/ConfigTest.scala 
0aca9385bff093b4cb52e42291c9c1d58d9600de 
  core/src/test/scala/unit/kafka/common/TopicTest.scala 
79532c89c41572ba953c4dc3319a05354927e961 
  core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala 
db5302ff02851f9f1a59419b1e071286bff0e142 
  core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala 
adf08010597b7c6ed72eddf93962497c3209e10f 
  core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala 
4b326d090c9486c812afb2603e94d77a2459d04c 
  core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 
359b0f5d14f82d14ee423cde271bb35b7034766d 
  
core/src/test/scala/unit/kafka/coordinator/ConsumerCoordinatorResponseTest.scala
 87a5330e716b9cedc9544229e29f42be7150c8fb 
  core/src/test/scala/unit/kafka/coordinator/ConsumerGroupMetadataTest.scala 
b69c993a3fa49f7f01dd28e13ce465c2a89eeba5 
  core/src/test/scala/unit/kafka/coordinator/CoordinatorMetadataTest.scala 
2cbf6e251adbfd3fb174d08138a13215c1914566 
  core/src/test/scala/unit/kafka/coordinator/PartitionAssignorTest.scala 
887cee5a582b5737ba838920399bb9b24bf22382 
  core/src/test/scala/unit/kafka/integration/AutoOffsetResetTest.scala 
139dc9a104c024e35fd9bc5ac9333e6bd208b571 
  core/src/test/scala/unit/kafka/integration/FetcherTest.scala 
facebd8f81c67f26f41a96bce343227bc9b55893 
  core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 
87c631573aa1e0275d6618c4ac3b33e76fa6abd3 
  core/src/test/scala/unit/kafka/integration/MinIsrConfigTest.scala 
a2c97134d85c637256440b5eb42f594d50f0cfe4 
  core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala 
6a758a7db71bdc4794b1e5f264f8ca6d410ff2ba 
  core/src/test/scala/unit/kafka/integration/ProducerConsumerTestHarness.scala 
4614a922e739098dbb0ff560d831e26045e32023 
  core/src/test/scala/unit/kafka/integration/RollingBounceTest.scala 
12d0733f5edf436315f884bc193da533d9d4a4ee 
  core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 
5b6c9d60d29436036f8287da6bad332c1a3a6ec9 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala 
e4bf2df48dd59a251b646b7f96d63ec4b924fc0b 
  
core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala
 74c761dec7afc98667032bdec8359a9aa7c2ecc2 
  core/src/test/scala/unit/kafka/javaapi/message/BaseMessageSetTestCases.scala 
726399e3c7a4157223b5037ff2a03da51236e876 
  core/src/test/scala/unit/kafka/javaapi/message/ByteBufferMessageSetTest.scala 
383fcef02994fde07e669651e522b9e5ee239dd8 
  core/src/test/scala/unit/kafka/log/CleanerTest.scala 
0e2a6a1e8e6d0bc6bed90cdc5bd3cb8e490ed364 
  core/src/test/scala/unit/kafka/log/FileMessageSetTest.scala 
02cf66882f4065f3ab8449650b0bd1b0e7525d38 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegration

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-16 Thread Guozhang Wang 

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


Could you rebase? Seems some previous commits get reflected in your latest 
patch.

- Guozhang Wang


On July 15, 2015, 11:58 p.m., Alexander Pakulov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35615/
> ---
> 
> (Updated July 15, 2015, 11:58 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1782
> https://issues.apache.org/jira/browse/KAFKA-1782
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' into KAFKA-1782
> 
> 
> Merge branch 'trunk' into KAFKA-1782
> 
> 
> KAFKA-1782; Cleanups
> 
> 
> KAFKA-1782; Cleanups
> 
> 
> Diffs
> -
> 
>   core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
> b0750faa43dfe21a9226335020b4b5dac6cf65bf 
>   core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
> 92ffb91b5e039dc0d4cd0e072ca46db32f280cf9 
>   core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
> afcc349342d44f42e19ca730dbd074c338638a64 
>   core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
> ce70a0a449883723a9b59ea48da34ba30b3f6daf 
>   core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
> 83de81cb3f79a6966dd5ef462733d8a22cd6d467 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
> ee94011894b46864614b97bbd2a98375a7d3f20b 
>   core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
> 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
>   core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
> 4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
>   core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
> df5c6ba20f01e497ce896af790cbab40369f1776 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala 
> 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 
>   core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
> 1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
> fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
>   core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
> dcd69881445c29765f66a7d21d2d18437f4df428 
>   core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
> 255442526d94157b7a0b5a92e1d6a900aacb7536 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
> 5717165f2344823fabe8f7cfafae4bb8af2d949a 
>   core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala 
> abe511fc1458bfb5a93c152aed81a827cc24ce68 
>   core/src/test/scala/unit/kafka/common/ConfigTest.scala 
> 0aca9385bff093b4cb52e42291c9c1d58d9600de 
>   core/src/test/scala/unit/kafka/common/TopicTest.scala 
> 79532c89c41572ba953c4dc3319a05354927e961 
>   core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala 
> db5302ff02851f9f1a59419b1e071286bff0e142 
>   core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala 
> adf08010597b7c6ed72eddf93962497c3209e10f 
>   core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala 
> 4b326d090c9486c812afb2603e94d77a2459d04c 
>   
> core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 
> 359b0f5d14f82d14ee423cde271bb35b7034766d 
>   
> core/src/test/scala/unit/kafka/coordinator/ConsumerCoordinatorResponseTest.scala
>  87a5330e716b9cedc9544229e29f42be7150c8fb 
>   core/src/test/scala/unit/kafka/coordinator/ConsumerGroupMetadataTest.scala 
> b69c993a3fa49f7f01dd28e13ce465c2a89eeba5 
>   core/src/test/scala/unit/kafka/coordinator/CoordinatorMetadataTest.scala 
> 2cbf6e251adbfd3fb174d08138a13215c1914566 
>   core/src/test/scala/unit/kafka/coordinator/PartitionAssignorTest.scala 
> 887cee5a582b5737ba838920399bb9b24bf22382 
>   core/src/test/scala/unit/kafka/integration/AutoOffsetResetTest.scala 
> 139dc9a104c024e35fd9bc5ac9333e6bd208b571 
>   core/src/test/scala/unit/kafka/integration/FetcherTest.scala 
> facebd8f81c67f26f41a96bce343227bc9b55893 
>   core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 
> 87c631573aa1e0275d6618c4ac3b33e76fa6abd3 
>   core/src/test/scala/unit/kafka/integration/MinIsrConfigTest.scala 
> a2c97134d85c637256440b5eb42f594d50f0cfe4 
>   core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala 
> 6a758a7db71bdc4794b1e5f264f8ca6d410ff2ba 
>   
> core/src/test/scala/unit/kafka/integration/ProducerConsumerTestHarness.scala 
> 4614a922e739098dbb0ff560d831e26045e32023 
>   core/src/test/scala/unit/kafka/integration/RollingBounceTest.scala 
> 12d0733f5edf436315f884bc193da533d9d4a4ee 
>   core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 
> 5b6c9d60d29436036f8287da6bad332c1a3a6ec9 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.sca

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-15 Thread Alexander Pakulov 


> On July 14, 2015, 2:49 a.m., Guozhang Wang wrote:
> > Some general comments:
> > 
> > 1. Regarding the @Before and @After annotations, one suggestion from the 
> > JIRA was that we remove any annotations other than "@Test" itself but use 
> > scalatest features (for example, 
> > http://doc.scalatest.org/2.2.4/#org.scalatest.BeforeAndAfter) instead. Now 
> > I cannot remember a strong motiviation for this move, so I feel it may be 
> > also OK as you chose to use the junit tags anyways.
> > 2. Regarding org.junit.Assert and org.scalatest.Assertions in imports, if 
> > we decide to be junit-heavy instead of scalatest-heavy for our unit tests, 
> > we should then use the former for most of the time and only the latter for 
> > intercept[..] since it is not supported in the fomer. There seems a few 
> > places where both of them are used for fail / assert, etc.

1. We could only use BeforeAndAfter for classes that aren't going to extended, 
because this trair isn't stackable.
I'll go ahead and try BeforeAndAfter each and report back with results.

2. Totally agree to keep code using only one.


> On July 14, 2015, 2:49 a.m., Guozhang Wang wrote:
> > core/src/test/scala/integration/kafka/api/ConsumerTest.scala, line 33
> > 
> >
> > Seems this import is not used inside the class?

I've found what was the root cause of this import I've accidentaly removed 
JUnit3Suite in KafkaServerTestHarness instead of replacing it with JUnitSuite. 
I'm going to remove unnecessary imports


> On July 14, 2015, 2:49 a.m., Guozhang Wang wrote:
> > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala, line 34
> > 
> >
> > Sometimes we use org.scalatest.Assertions.fail() and sometimes we use 
> > org.junit.Assert.fail(); it would better that we are consistent in one of 
> > them, and personally I recommend following the org.junit package since it 
> > is more general.

Fixed to use org.junit.* only


- Alexander


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


On July 15, 2015, 11:58 p.m., Alexander Pakulov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35615/
> ---
> 
> (Updated July 15, 2015, 11:58 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1782
> https://issues.apache.org/jira/browse/KAFKA-1782
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> Merge branch 'trunk' into KAFKA-1782
> 
> 
> Merge branch 'trunk' into KAFKA-1782
> 
> 
> KAFKA-1782; Cleanups
> 
> 
> KAFKA-1782; Cleanups
> 
> 
> Diffs
> -
> 
>   core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
> b0750faa43dfe21a9226335020b4b5dac6cf65bf 
>   core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
> 92ffb91b5e039dc0d4cd0e072ca46db32f280cf9 
>   core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
> afcc349342d44f42e19ca730dbd074c338638a64 
>   core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
> ce70a0a449883723a9b59ea48da34ba30b3f6daf 
>   core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
> 83de81cb3f79a6966dd5ef462733d8a22cd6d467 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
> ee94011894b46864614b97bbd2a98375a7d3f20b 
>   core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
> 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
>   core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
> 4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
>   core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
> df5c6ba20f01e497ce896af790cbab40369f1776 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala 
> 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 
>   core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
> 1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
> fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
>   core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
> dcd69881445c29765f66a7d21d2d18437f4df428 
>   core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
> 255442526d94157b7a0b5a92e1d6a900aacb7536 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
> 5717165f2344823fabe8f7cfafae4bb8af2d949a 
>   core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala 
> abe511fc1458bfb5a93c152aed81a827cc24ce68 
>   core/src/test/scala/unit/kafka/common/ConfigTest.scala 
> 0aca9385bff093b4cb52e42291c9c1d58d9600de 
>   core/src/test/scala/unit/kafka/common/TopicTest.scala 
> 79532c89c41572ba953c4dc3319a05354927e961

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-15 Thread Alexander Pakulov 

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

(Updated July 15, 2015, 11:58 p.m.)


Review request for kafka.


Bugs: KAFKA-1782
https://issues.apache.org/jira/browse/KAFKA-1782


Repository: kafka


Description (updated)
---

Merge branch 'trunk' into KAFKA-1782


Merge branch 'trunk' into KAFKA-1782


KAFKA-1782; Cleanups


KAFKA-1782; Cleanups


Diffs (updated)
-

  core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
b0750faa43dfe21a9226335020b4b5dac6cf65bf 
  core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
92ffb91b5e039dc0d4cd0e072ca46db32f280cf9 
  core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
afcc349342d44f42e19ca730dbd074c338638a64 
  core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
ce70a0a449883723a9b59ea48da34ba30b3f6daf 
  core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
83de81cb3f79a6966dd5ef462733d8a22cd6d467 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
ee94011894b46864614b97bbd2a98375a7d3f20b 
  core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
  core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
  core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
df5c6ba20f01e497ce896af790cbab40369f1776 
  core/src/test/scala/unit/kafka/admin/AdminTest.scala 
252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 
  core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
  core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
dcd69881445c29765f66a7d21d2d18437f4df428 
  core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
255442526d94157b7a0b5a92e1d6a900aacb7536 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
5717165f2344823fabe8f7cfafae4bb8af2d949a 
  core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala 
abe511fc1458bfb5a93c152aed81a827cc24ce68 
  core/src/test/scala/unit/kafka/common/ConfigTest.scala 
0aca9385bff093b4cb52e42291c9c1d58d9600de 
  core/src/test/scala/unit/kafka/common/TopicTest.scala 
79532c89c41572ba953c4dc3319a05354927e961 
  core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala 
db5302ff02851f9f1a59419b1e071286bff0e142 
  core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala 
adf08010597b7c6ed72eddf93962497c3209e10f 
  core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala 
4b326d090c9486c812afb2603e94d77a2459d04c 
  core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 
359b0f5d14f82d14ee423cde271bb35b7034766d 
  
core/src/test/scala/unit/kafka/coordinator/ConsumerCoordinatorResponseTest.scala
 87a5330e716b9cedc9544229e29f42be7150c8fb 
  core/src/test/scala/unit/kafka/coordinator/ConsumerGroupMetadataTest.scala 
b69c993a3fa49f7f01dd28e13ce465c2a89eeba5 
  core/src/test/scala/unit/kafka/coordinator/CoordinatorMetadataTest.scala 
2cbf6e251adbfd3fb174d08138a13215c1914566 
  core/src/test/scala/unit/kafka/coordinator/PartitionAssignorTest.scala 
887cee5a582b5737ba838920399bb9b24bf22382 
  core/src/test/scala/unit/kafka/integration/AutoOffsetResetTest.scala 
139dc9a104c024e35fd9bc5ac9333e6bd208b571 
  core/src/test/scala/unit/kafka/integration/FetcherTest.scala 
facebd8f81c67f26f41a96bce343227bc9b55893 
  core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 
87c631573aa1e0275d6618c4ac3b33e76fa6abd3 
  core/src/test/scala/unit/kafka/integration/MinIsrConfigTest.scala 
a2c97134d85c637256440b5eb42f594d50f0cfe4 
  core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala 
6a758a7db71bdc4794b1e5f264f8ca6d410ff2ba 
  core/src/test/scala/unit/kafka/integration/ProducerConsumerTestHarness.scala 
4614a922e739098dbb0ff560d831e26045e32023 
  core/src/test/scala/unit/kafka/integration/RollingBounceTest.scala 
12d0733f5edf436315f884bc193da533d9d4a4ee 
  core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 
5b6c9d60d29436036f8287da6bad332c1a3a6ec9 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala 
e4bf2df48dd59a251b646b7f96d63ec4b924fc0b 
  
core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala
 74c761dec7afc98667032bdec8359a9aa7c2ecc2 
  core/src/test/scala/unit/kafka/javaapi/message/BaseMessageSetTestCases.scala 
726399e3c7a4157223b5037ff2a03da51236e876 
  core/src/test/scala/unit/kafka/javaapi/message/ByteBufferMessageSetTest.scala 
383fcef02994fde07e669651e522b9e5ee239dd8 
  core/src/test/scala/unit/kafka/log/CleanerTest.scala 
0e2a6a1e8e6d0bc6bed90cdc5bd3cb8e490ed364 
  core/src/test/scala/unit/kafka/log/FileMessageSetTest.scala 
02cf6

Re: Review Request 35615: Patch for KAFKA-1782

2015-07-13 Thread Guozhang Wang 

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


Some general comments:

1. Regarding the @Before and @After annotations, one suggestion from the JIRA 
was that we remove any annotations other than "@Test" itself but use scalatest 
features (for example, 
http://doc.scalatest.org/2.2.4/#org.scalatest.BeforeAndAfter) instead. Now I 
cannot remember a strong motiviation for this move, so I feel it may be also OK 
as you chose to use the junit tags anyways.
2. Regarding org.junit.Assert and org.scalatest.Assertions in imports, if we 
decide to be junit-heavy instead of scalatest-heavy for our unit tests, we 
should then use the former for most of the time and only the latter for 
intercept[..] since it is not supported in the fomer. There seems a few places 
where both of them are used for fail / assert, etc.


core/src/test/scala/integration/kafka/api/ConsumerTest.scala (line 33)


Seems this import is not used inside the class?



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
(line 22)


Seems this import is not used as well. BTW I have another general comment 
regarding this issue.



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
(lines 263 - 266)


Is this intentional, as we already import org.junit.Assert._?



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
(lines 289 - 294)


Same as above.



core/src/test/scala/integration/kafka/api/ProducerSendTest.scala (line 34)


Sometimes we use org.scalatest.Assertions.fail() and sometimes we use 
org.junit.Assert.fail(); it would better that we are consistent in one of them, 
and personally I recommend following the org.junit package since it is more 
general.



core/src/test/scala/unit/kafka/admin/AdminTest.scala (lines 29 - 30)


Since we already imported Assert._ we do not need to import the other 
Assertions._ for this class.



core/src/test/scala/unit/kafka/network/SocketServerTest.scala (line 35)


Shall we import import org.junit.{After, Before, Test} instead of 
org.scalatest.junit.JUnitSuite?



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (lines 26 - 34)


Could you group the imports of org.*, java.*, kafka.*, etc together? Same 
for some other places.



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 118)


I think we can just use org.junit.Assert.fail here.



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 131)


same above.



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 144)


same above.



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 203)


same above.



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 263)


same above



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 297)


same above



core/src/test/scala/unit/kafka/producer/ProducerTest.scala (line 311)


same above


- Guozhang Wang


On June 18, 2015, 6:53 p.m., Alexander Pakulov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35615/
> ---
> 
> (Updated June 18, 2015, 6:53 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1782
> https://issues.apache.org/jira/browse/KAFKA-1782
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-1782; Junit3 Misusage
> 
> 
> Diffs
> -
> 
>   core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
> f56096b826bdbf760411a54ba067a6a83eca8a10 
>   core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
> 17b17b9b1520c7cc2e2ba96cdb1f9ff06e47bcad 
>   core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
> 07b1ff47bfc3cd3f948c9533c8dc977fa36d996f 
>   core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
> ce70a0a449883723a9b59ea48da34ba30b3f6daf 
>   core/src/test/scala/integration/kafka/api/ProducerCompress

Re: Review Request 35615: Patch for KAFKA-1782

2015-06-18 Thread Alexander Pakulov 

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

(Updated June 18, 2015, 6:53 p.m.)


Review request for kafka.


Bugs: KAFKA-1782
https://issues.apache.org/jira/browse/KAFKA-1782


Repository: kafka


Description
---

KAFKA-1782; Junit3 Misusage


Diffs (updated)
-

  core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
f56096b826bdbf760411a54ba067a6a83eca8a10 
  core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
17b17b9b1520c7cc2e2ba96cdb1f9ff06e47bcad 
  core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
07b1ff47bfc3cd3f948c9533c8dc977fa36d996f 
  core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
ce70a0a449883723a9b59ea48da34ba30b3f6daf 
  core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
83de81cb3f79a6966dd5ef462733d8a22cd6d467 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
ee94011894b46864614b97bbd2a98375a7d3f20b 
  core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
  core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
4764c8976022dfaaebd7ef1cd4ddca55a3ed8a89 
  core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
df5c6ba20f01e497ce896af790cbab40369f1776 
  core/src/test/scala/unit/kafka/admin/AdminTest.scala 
efb2f8e79b3faef78722774b951fea828cd50374 
  core/src/test/scala/unit/kafka/admin/DeleteConsumerGroupTest.scala 
1913ad6d3e6eb29a0c939c4f59f6b688c8c925fa 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 
fa8ce259a2832ab86f9dda8c1d409b2c42d43ae9 
  core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala 
c7136f20972614ac47aa57ab13e3c94ef775a4b7 
  core/src/test/scala/unit/kafka/api/ApiUtilsTest.scala 
255442526d94157b7a0b5a92e1d6a900aacb7536 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala 
5717165f2344823fabe8f7cfafae4bb8af2d949a 
  core/src/test/scala/unit/kafka/cluster/BrokerEndPointTest.scala 
abe511fc1458bfb5a93c152aed81a827cc24ce68 
  core/src/test/scala/unit/kafka/common/ConfigTest.scala 
0aca9385bff093b4cb52e42291c9c1d58d9600de 
  core/src/test/scala/unit/kafka/common/TopicTest.scala 
79532c89c41572ba953c4dc3319a05354927e961 
  core/src/test/scala/unit/kafka/consumer/ConsumerIteratorTest.scala 
db5302ff02851f9f1a59419b1e071286bff0e142 
  core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala 
adf08010597b7c6ed72eddf93962497c3209e10f 
  core/src/test/scala/unit/kafka/consumer/TopicFilterTest.scala 
4f124af5c3e946045a78ad1519c37372a72c8985 
  core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 
359b0f5d14f82d14ee423cde271bb35b7034766d 
  core/src/test/scala/unit/kafka/coordinator/ConsumerGroupMetadataTest.scala 
b69c993a3fa49f7f01dd28e13ce465c2a89eeba5 
  core/src/test/scala/unit/kafka/coordinator/CoordinatorMetadataTest.scala 
08854c5e6ec249368206298b2ac2623df18f266a 
  core/src/test/scala/unit/kafka/coordinator/PartitionAssignorTest.scala 
887cee5a582b5737ba838920399bb9b24bf22382 
  core/src/test/scala/unit/kafka/integration/AutoOffsetResetTest.scala 
139dc9a104c024e35fd9bc5ac9333e6bd208b571 
  core/src/test/scala/unit/kafka/integration/FetcherTest.scala 
facebd8f81c67f26f41a96bce343227bc9b55893 
  core/src/test/scala/unit/kafka/integration/KafkaServerTestHarness.scala 
87c631573aa1e0275d6618c4ac3b33e76fa6abd3 
  core/src/test/scala/unit/kafka/integration/MinIsrConfigTest.scala 
a2c97134d85c637256440b5eb42f594d50f0cfe4 
  core/src/test/scala/unit/kafka/integration/PrimitiveApiTest.scala 
6a758a7db71bdc4794b1e5f264f8ca6d410ff2ba 
  core/src/test/scala/unit/kafka/integration/ProducerConsumerTestHarness.scala 
4614a922e739098dbb0ff560d831e26045e32023 
  core/src/test/scala/unit/kafka/integration/RollingBounceTest.scala 
12d0733f5edf436315f884bc193da533d9d4a4ee 
  core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 
995b05901491bb0dbf0df210d44bd1d7f66fdc82 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala 
e4bf2df48dd59a251b646b7f96d63ec4b924fc0b 
  
core/src/test/scala/unit/kafka/javaapi/consumer/ZookeeperConsumerConnectorTest.scala
 74c761dec7afc98667032bdec8359a9aa7c2ecc2 
  core/src/test/scala/unit/kafka/javaapi/message/BaseMessageSetTestCases.scala 
726399e3c7a4157223b5037ff2a03da51236e876 
  core/src/test/scala/unit/kafka/javaapi/message/ByteBufferMessageSetTest.scala 
383fcef02994fde07e669651e522b9e5ee239dd8 
  core/src/test/scala/unit/kafka/log/CleanerTest.scala 
8b8249a35322a60ca94cb385a6cad25943dd1cc9 
  core/src/test/scala/unit/kafka/log/FileMessageSetTest.scala 
cec1caecc51507ae339ebf8f3b8a028b12a1a056 
  core/src/test/scala/unit/kafka/log/LogCleanerIntegrationTest.scala 
471ddff9bff1bdfa277c071e59e5c6b749b9c74f 
  core/src/test/scala/unit/kafka/log/LogConfigTest.scala 
3fd5a53f9b0edc0a7a169a185

Re: Review Request 35615: Patch for KAFKA-1782

2015-06-18 Thread Alexander Pakulov 

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

(Updated June 18, 2015, 6:39 p.m.)


Review request for kafka.


Bugs: KAFKA-1782
https://issues.apache.org/jira/browse/KAFKA-1782


Repository: kafka


Description (updated)
---

KAFKA-1782; Junit3 Misusage


Diffs
-

  clients/src/main/java/org/apache/kafka/clients/Metadata.java 
07f1cdb1fe920b0c7a5f2d101ddc40c689e1b247 
  clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java 
1e905240e7459a4a0a0573ae5d8ac19217cae197 
  clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
d1d1ec178f60dc47d408f52a89e52886c1a093a2 
  
clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java
 c1496a0851526f3c7d3905ce4bdff2129c83a6c1 
  clients/src/main/java/org/apache/kafka/clients/producer/MockProducer.java 
e66491cc82f11641df6516e7d7abb4a808c27368 
  clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java 
07e65d4a54ba4eef5b787eba3e71cbe9f6a920bd 
  clients/src/main/java/org/apache/kafka/common/network/Selector.java 
effb1e63081ed2c1fff6d08d4ecdf8a3cb43e40a 
  clients/src/main/java/org/apache/kafka/common/requests/AbstractRequest.java 
5e5308ec0e333179a9abbf4f3b750ea25ab57967 
  
clients/src/main/java/org/apache/kafka/common/requests/ConsumerMetadataRequest.java
 04b90bfe62456a6739fe0299f1564dbd1850fe58 
  clients/src/main/java/org/apache/kafka/common/requests/FetchRequest.java 
8686d83aa52e435c6adafbe9ff4bd1602281072a 
  clients/src/main/java/org/apache/kafka/common/requests/HeartbeatRequest.java 
51d081fa296fd7c170a90a634d432067afcfe772 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupRequest.java 
6795682258e6b329cc3caa245b950b4dbcf0cf45 
  clients/src/main/java/org/apache/kafka/common/requests/JoinGroupResponse.java 
8d418cd24cf6d105e9687a4a2492b8ed13738338 
  clients/src/main/java/org/apache/kafka/common/requests/ListOffsetRequest.java 
19267ee8aad5a2f5a84cecd6fc563f00329d5035 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java 
7e0ce159a2ddd041fc06116038bd5831bbca278b 
  clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java 
44e2ce61899889601b6aee71fa7f7ddb4a65a255 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 
8bf6cbb79a92b0878096e099ec9169d21e6d7023 
  
clients/src/main/java/org/apache/kafka/common/requests/OffsetFetchRequest.java 
deec1fa480d5a5c5884a1c007b076aa64e902472 
  clients/src/main/java/org/apache/kafka/common/requests/ProduceRequest.java 
fabeae3083a8ea55cdacbb9568f3847ccd85bab4 
  clients/src/test/java/org/apache/kafka/clients/MetadataTest.java 
928087d29deb80655ca83726c1ebc45d76468c1f 
  
clients/src/test/java/org/apache/kafka/clients/consumer/internals/CoordinatorTest.java
 b06c4a73e2b4e9472cd772c8bc32bf4a29f431bb 
  clients/src/test/java/org/apache/kafka/clients/producer/MockProducerTest.java 
6372f1a7f7f77d96ba7be05eb927c004f7fefb73 
  clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 
d23b4b6a7060eeefa9f47f292fd818c881d789c1 
  
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java 
e3cc1967e407b64cc734548c19e30de700b64ba8 
  clients/src/test/java/org/apache/kafka/test/MockSerializer.java 
e75d2e4e58ae0cdbe276d3a3b652e47795984791 
  core/src/main/scala/kafka/cluster/EndPoint.scala 
e9008e6d758be04bebe3cc70952c13dc55dc58fb 
  core/src/main/scala/kafka/log/LogConfig.scala 
a907da09e1ccede3b446459225e407cd1ae6d8b3 
  core/src/main/scala/kafka/network/RequestChannel.scala 
357d8b97cb336857500213efade77950833c2096 
  core/src/main/scala/kafka/server/DelayedOperation.scala 
123078d97a7bfe2121655c00f3b2c6af21c53015 
  core/src/main/scala/kafka/server/KafkaApis.scala 
d63bc18d795a6f0e6994538ca55a9a46f7fb8ffd 
  core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 
b31b432a226ba79546dd22ef1d2acbb439c2e9a3 
  core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala 
f56096b826bdbf760411a54ba067a6a83eca8a10 
  core/src/test/scala/integration/kafka/api/ConsumerTest.scala 
17b17b9b1520c7cc2e2ba96cdb1f9ff06e47bcad 
  core/src/test/scala/integration/kafka/api/IntegrationTestHarness.scala 
07b1ff47bfc3cd3f948c9533c8dc977fa36d996f 
  core/src/test/scala/integration/kafka/api/ProducerBounceTest.scala 
ce70a0a449883723a9b59ea48da34ba30b3f6daf 
  core/src/test/scala/integration/kafka/api/ProducerCompressionTest.scala 
83de81cb3f79a6966dd5ef462733d8a22cd6d467 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 
ee94011894b46864614b97bbd2a98375a7d3f20b 
  core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 
  core/src/test/scala/unit/kafka/KafkaConfigTest.scala 
4cb92de169d465ccdafb8c24b5b46b0f259c8e43 
  core/src/test/scala/unit/kafka/admin/AddPartition