> On March 31, 2015, 9:20 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 1 > > <https://reviews.apache.org/r/30809/diff/4/?file=903374#file903374line1> > > > > 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).
Will do. > On March 31, 2015, 9:20 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 49 > > <https://reviews.apache.org/r/30809/diff/4/?file=903374#file903374line49> > > > > 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. I wanted to have two (topic, partition) tuples with leader on each broker. I have decided to use a single topic with multiple partitions rather than using two topics which could have also worked. The reason for picking the first approach was that essentially if I wanted to leverage continuous validation test outside of system test framework with in a test cluster with other topics. In order to illustrate why the second approach won't work in that scenario is that if we have 3 brokers with one partition if I create 3 topics (T1P1, T2P1, T3P1) then the following would be a valid assignment based on existing broker assignment algorithm. B1 B2 B3 T1P1 TXP1 TXP2 T2P1 TYP1 TYP2 T3P1 where TX and TY are other production topics running in that cluster. In this case all the leaders have landed on the same broker. However the first approach precludes this possibility. The file signalling was to workaround the fact that the most commonly used client does not have capability to consume from a particular partition. The way I have set it up the file signalling acts as a barrier. We make sure all the producer/consumer pairs have been instantiated with the hope being that they have talked to zookeeper and reserved their parition. Once both the consumers have been instantiated we expect themselves to have bound themselves to a particular partition we can now let the producers run in both the instances and this way we are assured that the consumer should never receive data from same producer. > On March 31, 2015, 9:20 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 52 > > <https://reviews.apache.org/r/30809/diff/4/?file=903374#file903374line52> > > > > 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 This is a poor man's rate limiter as compared to guava rate limiter. I will make it private. > On March 31, 2015, 9:20 p.m., Joel Koshy wrote: > > system_test/broker_upgrade/bin/test-broker-upgrade.sh, line 1 > > <https://reviews.apache.org/r/30809/diff/4/?file=903376#file903376line1> > > > > 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. The continuous validation test will be useful outside of the system test framework. This was an attempt to leverage CVT in the system test setting. I think since strong objections have been raised against adopting this approach I will leave a comment on this patch accordingly. - Abhishek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30809/#review78270 ----------------------------------------------------------- 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 > >
