> On March 11, 2015, 11:12 p.m., Gwen Shapira wrote: > > This looks like a very good start. I think the framework is flexible enough > > to allow us to add a variety of upgrade tests. I'm looking forward to it. > > > > > > I have few comments, but mostly I'm still confused on how this will be > > used. Perhaps more comments or even a readme is in order > > > > You wrote that we invoke "test.sh <dir1> <dir2>", what should each > > directory contain? just the kafka jar of different versions? or an entire > > installation (including bin/ and conf/)? > > Which one of the directories should be the newer and which is older? does > > it matter? > > Which version of clients will be used. > > > > Perhaps a more descriptive name for test.sh can help too. I'm guessing > > we'll have a whole collection of those test scripts soon. > > > > Gwen
The directory containing the kafka jars. kafka_2.10-0.8.3-SNAPSHOT.jar kafka-clients-0.8.3-SNAPSHOT.jar The other jars are shared between both the kafka brokers. > On March 11, 2015, 11:12 p.m., Gwen Shapira wrote: > > build.gradle, line 209 > > <https://reviews.apache.org/r/30809/diff/3/?file=889854#file889854line209> > > > > This should probably be a test dependency (if needed at all) > > > > Packaging Guava will be a pain, since so many systems use different > > versions of Guava and they are all incompatible. Guava provides an excellent rate limiter which I am using in the test and have used in the past. When you talk about packaging we are already pulling in other external libraries like zookeeper with a specific version which the applications might be using extensively and might similarly run into conflicts. If you have a suggestion for a library which provides rate limiting(less popular) than guava then I can use that instead otherwise I will move this dependency to the test for now. > On March 11, 2015, 11:12 p.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, lines 409-440 > > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line409> > > > > Do we really want to do this? > > > > We are using joptsimple for a bunch of other tools. It is easier to > > read, maintain, nice error messages, help screen, etc. Thanks, I will switch to jobOpts. > On March 11, 2015, 11:12 p.m., Gwen Shapira wrote: > > system_test/broker_upgrade/bin/kafka-run-class.sh, lines 152-156 > > <https://reviews.apache.org/r/30809/diff/3/?file=889856#file889856line152> > > > > Why did we decide to duplicate this entire file? The only difference is that it takes an additional argument which contains the directory from which the kafka jars should be pulled. Would you recommend adding it to the original script as an optional argument? - Abhishek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30809/#review76157 ----------------------------------------------------------- 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 > >
