You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Abhishek Nigam <an...@linkedin.com> on 2015/02/10 00:41:31 UTC
Review Request 30809: Patch for KAFKA-1888
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1888
https://issues.apache.org/jira/browse/KAFKA-1888
Repository: kafka
Description
-------
Cleaning up the scripts, forgot to add build file pulling in guava library
Fixing build.gradle
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
-------
Thanks,
Abhishek Nigam
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review76835
-----------------------------------------------------------
Also, I think we can move ValidatingProducer/Consumer and BoostrapConsumer to kafka.perf if we could do the refactoring based on Producer/ConsumerPerformance.
- 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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review73061
-----------------------------------------------------------
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment119244>
Flip is needed to reset the pointer to beginning of byte buffer.
- Abhishek Nigam
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Guozhang Wang <wa...@gmail.com>.
> On March 10, 2015, 11:44 p.m., Guozhang Wang wrote:
> >
This is from some old review comments, I will upload rest of them soon.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review73382
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review73382
-----------------------------------------------------------
build.gradle
<https://reviews.apache.org/r/30809/#comment119686>
Additional dependency:
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment119687>
Better have specific imports than wildcards.
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment119689>
"..Test-" + time?
- 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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> 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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review76157
-----------------------------------------------------------
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
build.gradle
<https://reviews.apache.org/r/30809/#comment123636>
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.
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment123635>
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.
system_test/broker_upgrade/bin/kafka-run-class.sh
<https://reviews.apache.org/r/30809/#comment123638>
Why did we decide to duplicate this entire file?
- Gwen Shapira
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> 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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> On April 2, 2015, 1:38 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, lines 431-437
> > <https://reviews.apache.org/r/30809/diff/4/?file=903374#file903374line431>
> >
> > Could we add a description of the test (what kind of data is generated, how does consumer to the verification, what kind of output is generated, etc)?
The data which is generated is very simple - increasing sequence of longs with timestamp. The producer keeps track of the newest sequence number, timestamp which it has sent.
The consumer keeps track of the last sequence number and timestamp which it has received. The system test will interrupt the CVT and compare the sequence numbers between the producer and the sender. If they do not line up then it is an error. (If either the producer or consumer threads terminate un-expectedly before they have been interrupted it will be flagged as an error) If the test fails then the data logs from the producer and consumer are not removed and can be inspected.
The idea behind putting the consumer and producer in the same JVM was orthogonal to system test and was in case it is used in a test cluster hosting other topics it makes easy to get hands on some things like delta etc. However, I think there is very strong objection to adopting this for system tests which are short-lived in nature. Unless there is support for the approach I have taken so far I plan to revert to the existing approach of spawning multiple JVMs for producer and consumer.
I will change the bash script to be in python similar to what other system tests do.
> On April 2, 2015, 1:38 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, lines 440-454
> > <https://reviews.apache.org/r/30809/diff/4/?file=903374#file903374line440>
> >
> > Could we add a description of each command line option?
I need to add more documentation. I will add this in.
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review78630
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review78630
-----------------------------------------------------------
Thanks for the patch. A few high level comments.
1. I think we are standardizing on python for test system tests. Should we write test-broker-upgrade.sh in python?
2. The existing system tests also generate loads and do verification. However, that part of logic is a bit hacky. For example, we hacked ProducerPerfomance a bit to generate data with sequential ids. Do we plan to replace those with ContinuousValidationTest in the future? Ideally, it would be great if we can write the common drive code (e.g., loading data, reading data, verifying) once and reuse it in all tests.
3. When doing upgrades, we typically upgrade the brokers first, followed by the clients. To simulate that, the clients need to run on an old version. Have you thought about how to do that?
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment127544>
Could we add a description of the test (what kind of data is generated, how does consumer to the verification, what kind of output is generated, etc)?
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment127543>
Could we add a description of each command line option?
- Jun Rao
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Ashish Singh <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review77732
-----------------------------------------------------------
bin/kafka-run-class.sh
<https://reviews.apache.org/r/30809/#comment125922>
Probably a leftover debug print.
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment125923>
Not sure if this belongs to tools package. Any specific reason why we cannot use existing producer consumer tests' variants? Can they be extended to avoid creating yet another producer consumer test?
system_test/broker_upgrade/bin/test-broker-upgrade.sh
<https://reviews.apache.org/r/30809/#comment125924>
Relying on greps for validation is probably not the best way.
system_test/broker_upgrade/bin/test-broker-upgrade.sh
<https://reviews.apache.org/r/30809/#comment125925>
Probably, extract out in a "upgrade" method for reusability and readability.
- Ashish Singh
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
-----------------------------------------------------------
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)
-------
Updated the RB with Gwen's comments, Beckett's comments and a subset of Guozhang's comments
Diffs (updated)
-----
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
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Jiangjie Qin <be...@gmail.com>.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 183
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line183>
> >
> > This is essentially a sync approach, can we use callback to do this?
>
> Abhishek Nigam wrote:
> This is intentional. We want to make sure the event has successfully reached the brokers. This enables us to form a reasonable expectation of what the consumer should expect.
The callback should be able to make sure everything goes well otherwise you can chose stop sending or do whatever you want. One big issue about this approach is that you will only send a single message out for each batch, and that would be very slow especially if you don's set linger time to be some thing very small even zero.
Ideally the test case should work with differnt producer settings, I would say at least ack=1 and ack=-1, also with different batch size and linger time. Sending a single message out for each batch does not look a very useful test case.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 184
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line184>
> >
> > When a send fails, should we at least log the sequence number?
>
> Abhishek Nigam wrote:
> I log the exception and the logger gives me the timestamp in the logs.
> Maybe I am missing something. Can you explain the rationale of why we would want to log the sequence number on the producer side when send fails.
Suppose someone is reading the log because something went wrong, wouldn't it be much faster to see how many and which messages are lost if you have sequence number logged?
For example, with sequence number, you can see producer saying that I messge 1,2,3 sent successfully while message 4 failed. And consumer would say, I expect to see 1,2,3 but I just got 1,3. 2 is lost.
In your current log, what you can see is just a message wasn't sent successfully, and one message was not consumed as expected. It's more complicated to debug, right?
- Jiangjie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review76173
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 183
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line183>
> >
> > This is essentially a sync approach, can we use callback to do this?
This is intentional. We want to make sure the event has successfully reached the brokers. This enables us to form a reasonable expectation of what the consumer should expect.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 184
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line184>
> >
> > When a send fails, should we at least log the sequence number?
I log the exception and the logger gives me the timestamp in the logs.
Maybe I am missing something. Can you explain the rationale of why we would want to log the sequence number on the producer side when send fails.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 321
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line321>
> >
> > Similar to producer, can we log the expected sequence number and the seq we actually saw?
Sure in the cases where this a mismatch I could do that.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 386
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line386>
> >
> > Can we use KafkaThread here?
I will take a look at that.
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review76173
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 183
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line183>
> >
> > This is essentially a sync approach, can we use callback to do this?
>
> Abhishek Nigam wrote:
> This is intentional. We want to make sure the event has successfully reached the brokers. This enables us to form a reasonable expectation of what the consumer should expect.
>
> Jiangjie Qin wrote:
> The callback should be able to make sure everything goes well otherwise you can chose stop sending or do whatever you want. One big issue about this approach is that you will only send a single message out for each batch, and that would be very slow especially if you don's set linger time to be some thing very small even zero.
> Ideally the test case should work with differnt producer settings, I would say at least ack=1 and ack=-1, also with different batch size and linger time. Sending a single message out for each batch does not look a very useful test case.
I think what you bring about ack=1 and ack=-1 is a very good point but orthogonal to this discussion. This can be a config paramter and we could have different configurations as you pointed out.
This is however not intended to be a performance test since there are other system tests which do that much better.
LeaderNotAvailableException and UnkownHostOrTopicException are actual exceptions that I see in my logs every time I ran this test and it does not make sense to stop the producer thread in this case.
To clarify the purpose of this test:
a) Data integrity - old consumer can pull from the new broker without any issues. There is no data format change which will cause issues for the old consumers consuming from new broker. (This is tested through bootstrap)
b) Data is received in order and this guarantee is not violated despite the rolling bounce.
> On March 12, 2015, 12:13 a.m., Jiangjie Qin wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 184
> > <https://reviews.apache.org/r/30809/diff/3/?file=889855#file889855line184>
> >
> > When a send fails, should we at least log the sequence number?
>
> Abhishek Nigam wrote:
> I log the exception and the logger gives me the timestamp in the logs.
> Maybe I am missing something. Can you explain the rationale of why we would want to log the sequence number on the producer side when send fails.
>
> Jiangjie Qin wrote:
> Suppose someone is reading the log because something went wrong, wouldn't it be much faster to see how many and which messages are lost if you have sequence number logged?
> For example, with sequence number, you can see producer saying that I messge 1,2,3 sent successfully while message 4 failed. And consumer would say, I expect to see 1,2,3 but I just got 1,3. 2 is lost.
> In your current log, what you can see is just a message wasn't sent successfully, and one message was not consumed as expected. It's more complicated to debug, right?
Since the sender re-tries the message till it succeeds I am not sure how we would arrive at the issue you are mentioning but maybe I am missing something.
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review76173
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Jiangjie Qin <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review76173
-----------------------------------------------------------
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment123652>
This is essentially a sync approach, can we use callback to do this?
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment123653>
When a send fails, should we at least log the sequence number?
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment123659>
Similar to producer, can we log the expected sequence number and the seq we actually saw?
core/src/main/scala/kafka/tools/ContinuousValidationTest.java
<https://reviews.apache.org/r/30809/#comment123655>
Can we use KafkaThread here?
- Jiangjie Qin
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
-----------------------------------------------------------
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 (updated)
-------
Fixing the tests based on Mayuresh comments, code cleanup after proper IDE setup
Diffs (updated)
-----
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
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/
-----------------------------------------------------------
(Updated Feb. 18, 2015, 1:59 a.m.)
Review request for kafka.
Bugs: KAFKA-1888
https://issues.apache.org/jira/browse/KAFKA-1888
Repository: kafka
Description (updated)
-------
patch for KAFKA-1888
Diffs (updated)
-----
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
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 168
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line168>
> >
> > same here can we use isInterrupted()?
> >
> > http://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html
I want to check if the current thread is interrupted. The link you sent out is useful if I wanted to query whether another thread was interrupted.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 280
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line280>
> >
> > Can we put this in a separate method like init().
> > Constructor can be used mainly for assignment. what do you think?
Moved the launching of the threads to an init method.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 298
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line298>
> >
> > When is the blockingCallInterrupted set to true?
Got rid of this.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 324
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line324>
> >
> > formatting spaces.
> >
> > Will there be a case where :
> > (evt.sequenceId < lastEventSeenSequenceId.get() && evt.eventProducedTimestamp > lastEventSeenTimeProduced.get()
This will happen when the sequence numbers wraparound.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 61
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line61>
> >
> > 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.
Fixed this.
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review72786
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 207
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line207>
> >
> > 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?
This will not be an issue since for timed runs we will interrupt the thread anyway after a fixed time. This is the mode
which is being used in the upgrade test.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 424
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line424>
> >
> > Are you assuming that first argument will be some key?
If you take a look at the script I am expecting alternate parameters like -timedRun <> -timeToSpawn <>
Key is essentially the parameter name.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 474
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line474>
> >
> > what do you mean by rebuild state later?
What I meant was that between two runs for the rolling upgrade test we will not re-use any state from zookeeper or the brokers so I do not need to worry about clean shutdown.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 77
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line77>
> >
> > Why we need a flip?
The flip is needed to reset the get pointer in byte buffer to beginning of the byte buffer else we will get underflow exception.
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review72786
-----------------------------------------------------------
On Feb. 18, 2015, 1:59 a.m., Abhishek Nigam wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30809/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 1:59 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1888
> https://issues.apache.org/jira/browse/KAFKA-1888
>
>
> Repository: kafka
>
>
> Description
> -------
>
> patch for KAFKA-1888
>
>
> 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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
> On Feb. 18, 2015, 12:06 a.m., Mayuresh Gharat wrote:
> > core/src/main/scala/kafka/tools/ContinuousValidationTest.java, line 400
> > <https://reviews.apache.org/r/30809/diff/1/?file=859055#file859055line400>
> >
> > 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.
IDE setup was a little messed up. I looked up kafka coding guidelines and there does not seem to be anything about comments so made the indenting consistent using the IDE.
- Abhishek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30809/#review72786
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Mayuresh Gharat <gh...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 30809: Patch for KAFKA-1888
Posted by Abhishek Nigam <an...@linkedin.com>.
-----------------------------------------------------------
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 (updated)
-------
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 (updated)
-------
Scripted it to run 20 times without any failures.
Command-line: broker-upgrade/bin/test.sh <dir1> <dir2>
Thanks,
Abhishek Nigam