----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30809/#review76103 -----------------------------------------------------------
There some coding convention suggestions that may not be comprehensive. You can take a look at these wiki pages: http://kafka.apache.org/coding-guide.html https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Error+Handling+and+Logging Also I think the ValidatingProducer / ValidatingConsumer can be implemented via extending ShutdownableThread and Producer/ConsumerPerformance, which will save lots of code here. build.gradle <https://reviews.apache.org/r/30809/#comment124483> I think the general idea is to not introduce external dependencies to core / clients dependencies, but only to test dependencies (i.e. testCompile) like yammer / jopt. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment123508> Could you group the imports with org.apache.kafka.. kafka.. java.. scala.. other external imports.. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment124484> The comments here are a bit too high-level, would better off adding some details about: 1) the consumer scenarios: real-time consumer and bootstrap consumer. 2) core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment123509> Explicitly specify these varialbes as public or private. For example, the logger should be: private static final Logger = .. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment124485> Rename to "ContinuousValidationTestRealTimeConsumer" and "ContinuousValidationTestBootstrapConsumer"? Also there are some inconsistency in the comments / code below between "consumer" and "real-time consumer" but they are actually referring to the same thing. Could you make sure the terms are consistent, e.g. "real-time consumer". core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment123511> Move the static variables to the top of the class. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment123513> Add a space after "//", ditto below. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment123512> Do not need to add the "//need flip" comment, as generally when the buffer swtich from write mode to read mode one needs to flip. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment124492> Could we move all the inner classes to the bottom of the ContinuousValidationTest class? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment124489> Could we use /** * */ for class-level comments? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment124490> Can we just extend ProducerPerformance to extend this class? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment124495> Unclear comments, and also below. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment124491> Likewise, can we extend ConsumerPerformance to implement this class? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment123514> 1. Move these declarations to the top of the class. 2. Rename consumer to realtimeConsumer. 3. I think we do not need to maintain a separate thread for each one of producer / realtimeConsumer / bootstrapConsumer. You could take a look at kafka.utils.ShutdownableThread, and implement, for example, ValidatingConsumer as class ValidatingConsumer extends ConsumerPerformance, ShutdownableThread.. system_test/broker_upgrade/bin/kafka-run-class.sh <https://reviews.apache.org/r/30809/#comment124497> Can we just use the the kafka-run-class.sh in bin? For example, in system_test/producer_perf/bin/run-compression-test.sh, you can see "$base_dir/../../bin/kafka-run-class.sh" - Guozhang Wang On March 9, 2015, 11:55 p.m., Abhishek Nigam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30809/ > ----------------------------------------------------------- > > (Updated March 9, 2015, 11:55 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1888 > https://issues.apache.org/jira/browse/KAFKA-1888 > > > Repository: kafka > > > Description > ------- > > Fixing the tests based on Mayuresh comments, code cleanup after proper IDE > setup > > > Diffs > ----- > > build.gradle 0f0fe60a74542efa91a0e727146e896edcaa38af > core/src/main/scala/kafka/tools/ContinuousValidationTest.java PRE-CREATION > system_test/broker_upgrade/bin/kafka-run-class.sh PRE-CREATION > system_test/broker_upgrade/bin/test.sh PRE-CREATION > system_test/broker_upgrade/configs/server1.properties PRE-CREATION > system_test/broker_upgrade/configs/server2.properties PRE-CREATION > system_test/broker_upgrade/configs/zookeeper_source.properties PRE-CREATION > > Diff: https://reviews.apache.org/r/30809/diff/ > > > Testing > ------- > > Scripted it to run 20 times without any failures. > Command-line: broker-upgrade/bin/test.sh <dir1> <dir2> > > > Thanks, > > Abhishek Nigam > >
