----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30809/#review78270 -----------------------------------------------------------
bin/kafka-run-class.sh <https://reviews.apache.org/r/30809/#comment126788> Can you remove this (and the echo that Ashish pointed out)? core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126790> This should definitely not be in tools - this should probably live somewhere under clients/test. I don't think those are currently exported though, so we will need to modify build.gradle. However, per other comments below I'm not sure this should be part of system tests since it is (by definition long running). core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126791> Can remove this core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126822> It would help a lot if you could add comments describing what validation is done. For e.g., I'm unclear on why we need the complicated file-based signaling mechanism. So a high-level description would help a lot. More importantly, I really think we should separate "continuous validation" from "broker upgrade" which is the focus of KAFKA-1888 In order to do a broker upgrade test, we don't need any additional code. We just instantiate the producer performance and consumer via system test utils. Keep those on the old jar. The cluster will start with the old jar as well and during the test we bounce in the latest jar (the system test utils will need to be updated to support this). We then do the standard system test validation - that all messages sent were received. core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126794> This appears to be for rate-limiting the producer but can be more general than that. It would help to add a comment describing its purpose. Also, should probably be private core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126793> 1 -> one core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126792> Stray println core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126795> Stray println core/src/main/scala/kafka/tools/ContinuousValidationTest.java <https://reviews.apache.org/r/30809/#comment126829> Can you use the logger formatter we use elsewhere? i.e., curly braces instead of an explicit String.format system_test/broker_upgrade/bin/test-broker-upgrade.sh <https://reviews.apache.org/r/30809/#comment127013> This appears to be a one-off script to set up the test. This needs to be done within the system test framework which already has a number of utilities that do similar things. One other comment is that the patch is for an upgrade test, but I think it is a bit confusing to mix this with CVT. system_test/broker_upgrade/bin/test-broker-upgrade.sh <https://reviews.apache.org/r/30809/#comment127010> may be better to just use mktemp for temporary files system_test/broker_upgrade/bin/test-broker-upgrade.sh <https://reviews.apache.org/r/30809/#comment127011> should these (and below) be in basedir? That said I don't see this created anywhere. - Joel Koshy On March 23, 2015, 6:54 p.m., Abhishek Nigam wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30809/ > ----------------------------------------------------------- > > (Updated March 23, 2015, 6:54 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1888 > https://issues.apache.org/jira/browse/KAFKA-1888 > > > Repository: kafka > > > Description > ------- > > Updated the RB with Gwen's comments, Beckett's comments and a subset of > Guozhang's comments > > > Diffs > ----- > > bin/kafka-run-class.sh 881f578a8f5c796fe23415b978c1ad35869af76e > core/src/main/scala/kafka/tools/ContinuousValidationTest.java PRE-CREATION > core/src/main/scala/kafka/utils/ShutdownableThread.scala > fc226c863095b7761290292cd8755cd7ad0f155c > system_test/broker_upgrade/bin/test-broker-upgrade.sh 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 > >
