----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30809/#review72786 -----------------------------------------------------------
core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118863> Can you remove this core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118889> Any reason for not making this final? static variables should come before Instance variables. Its a common standard to specify instance variables with _ like : _groupId. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118890> same here : any reason for not making it final? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118913> Why we need a flip? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118940> same here can we use isInterrupted()? http://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118933> This might end up in infinite loop if something goes wrong with cluster, right? Should we have a maximum numnber of retries? What do you think? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118941> You might want to do : Thread.currentThread().interrupt() . Then you might not require the blockingCallInterrupted flag : http://www.ibm.com/developerworks/library/j-jtp05236/ http://www.javamex.com/tutorials/threads/thread_interruption_2.shtml What do you think? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118911> Can we put this in a separate method like init(). Constructor can be used mainly for assignment. what do you think? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118915> When is the blockingCallInterrupted set to true? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118939> Thread.interrupted resets the interrupt flag. Can we use isInterrupted()? http://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118925> formatting spaces. Will there be a case where : (evt.sequenceId < lastEventSeenSequenceId.get() && evt.eventProducedTimestamp > lastEventSeenTimeProduced.get() core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118885> The common format of commenting is : // this is a comment Personally I don't mind, but thats kind of a standard that I understood from the reviews that I got. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118893> Are you assuming that first argument will be some key? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment118907> what do you mean by rebuild state later? - Mayuresh Gharat On Feb. 9, 2015, 11:53 p.m., Abhishek Nigam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30809/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2015, 11:53 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1888 > https://issues.apache.org/jira/browse/KAFKA-1888 > > > Repository: kafka > > > Description > ------- > > Essentially this test does the following: > a) Start a java process with 3 threads > Producer - producing continuously > Consumer - consuming from latest > Bootstrap consumer - started after a pause to bootstrap from beginning. > > It uses sequentially increasing numbers and timestamps to make sure we are > not receiving out of order messages and do real-time validation. > > b) Script which wraps this and takes two directories which contain the kafka > version specific jars: > kafka_2.10-0.8.3-SNAPSHOT-test.jar > kafka_2.10-0.8.3-SNAPSHOT.jar > > The first argument is the directory containing the older version of the jars. > The second argument is the directory containing the newer version of the jars. > > The reason for choosing directories was because there are two jars in these > directories: > > > Diffs > ----- > > build.gradle c3e6bb839ad65c512c9db4695d2bb49b82c80da5 > 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 > >
