KAFKA-12459; Use property testing library for raft event simulation tests #10323

hachikuji · 2021-03-15T23:11:01Z

This patch changes the raft simulation tests to use jqwik, which is a property testing library. This provides two main benefits:

It simplifies the randomization of test parameters. Currently the tests use a fixed set of Random seeds, which means that most builds are doing redundant work. We get a bigger benefit from allowing each build to test different parameterizations.
It makes it easier to reproduce failures. Whenever a test fails, jqwik will report the random seed that failed. A developer can then modify the @Property annotation to use that specific seed in order to reproduce the failure.

Note that I have resisted making logical changes to the tests themselves. The only difference is the way the parameters are specified.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…ests

dajac

This is pretty cool framework! The PR LGTM. I just left a minor suggestion.

raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java

chia7712

Run this patch with junit 5.8.0-M1. works well so +1

chia7712 · 2021-03-16T08:57:33Z

raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java

@@ -525,33 +430,27 @@ void schedule(Runnable action, int delayMs, int periodMs, int jitterMs) {
        }

        void runUntil(Supplier<Boolean> exitCondition) {
-            while (!exitCondition.get()) {
-                if (queue.isEmpty())
+            for (int iteration = 0; iteration < MAX_ITERATIONS; iteration++) {


How about setting @Timeout instead of MAX_ITERATIONS?

ijuma · 2021-03-16T14:05:57Z

Nice! How did we pick jqwik?

hachikuji · 2021-03-16T17:36:43Z

I can't claim to have searched all the library options extensively. I found out about jqwik from @stanislavkozlovski. The documentation seemed clear and it took almost no time to get something working. I do like how extensible the framework seems. I thought I might need some of that here, but it turned out to be a pretty simple usage within the standard API.

ijuma · 2021-03-16T17:52:57Z

@hachikuji Sounds good. Can we take a quick look at the two alternatives mentioned in the following blog post?
https://blog.johanneslink.net/2020/01/28/why-is-pbt-not-popular-in-java/

I think you choose the right one, but good to do a bit of due diligence as future Java-based property based tests will rely on whatever we pick here.

stanislavkozlovski · 2021-03-16T18:04:10Z

For what it's worth I evaluated jqwik vs Quickcheck and had the following bullet-point summaries:

Quickcheck

819 stars
10-year old library
v1.0 released Nov 22, 2020
25 open issues
55 commits in the last year
MIT License
light documentation
supports shrinking

Jqwik

260 stars
5-year old library
v1.5 release Feb 2021
16 open issues
1039 commits in the last year
EPL-2.0 License
extensive documentation
very configurable
supports shrinking

My reasoning to prefer Jqwik was that it seemed more actively maintained, had good interfaces, had very extensive documentation (I value this heavily) and most importantly supports programmatic parameter generation, meaning it allows you to easily express the dependencies of randomized input. I got the notion that this random input dependency generation is one of the trickier things when writing more complex test cases from this blog post.

Jqwik has some other interesting features like collecting and reporting statstics on the data it generates, allowing you to inspect what the generated data is and whether it's useful or can be tweaked.

ijuma · 2021-03-16T19:13:53Z

Thanks, this is very helpful. Since this is EPL 2, we need to check https://apache.org/legal/resolved.html#weak-copyleft-licenses

hachikuji · 2021-03-17T16:30:34Z

@ijuma Thanks for the link. EPL 2.0 is listed as a "weak copyleft" license, similar to Jersey under CDDL. The document says that we can include a binary dependence as long as it is appropriately labeled. I've updated the NOTICE file with language similar to the note about Jersey.

chia7712 · 2021-03-17T18:40:11Z

unrelated to this PR. forRandomRunning is not used. Maybe we can remove it in this PR? Also, random of RaftNode is not used also.

hachikuji · 2021-03-17T18:45:54Z

@chia7712 I removed forRandomRunning here: #10309.

jsancio

Very cool. Excited to use this in other tests.

jsancio · 2021-03-17T21:53:40Z

raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java

-
-            scheduler.runUntil(() -> cluster.allReachedHighWatermark(20, nonPartitionedNodes));
-        }
+    @Property(tries = 100)


Hmm. These simulation tests are already the slowest suite in raft:test. I git fetch the PR and it looks like they still run when calling ./gradlew raft:test. Is there away to set this to a smaller number (2) as the default and override it if the user wants to run them with 100 tries?

Btw, 5 (number of possible voters) * 6 (number of possible observers) is 30 which is much smaller than 100. I assume that the library will run the same configuration with multiple seeds.

The PR is reducing the number of runs by quite a lot. I justified this on account of the randomized seed which allow more of the builds to be doing useful (i.e. non-redundant) work. I think there are a bunch of improvements we can make to improve execution time overall. Also I suspect there was a regression since these used to execute much faster. Really I'd like to get to the point where we can run thousands of simulations on each build without significantly adding build time.

I was inspired to take a look and found the big regression. The way we catch NoSuchElementException in MockLog was killing the performance of these tests because of need to fill in the stack trace. I tested locally with a fix and it brought the local time for this test from over 4 minutes to 48 seconds. On trunk, this test is taking more than 16 minutes for me, so overall this patch will be a major improvement.

Very cool. I made the same change to KafkaMetadataLog: #10344

jsancio · 2021-03-17T21:56:23Z

raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java

-        }
+    @Property(tries = 100)
+    void canElectInitialLeader(
+        @ForAll Random random,


I assume that @ForAll will cause the library to run this test multiple times with different random seeds. If so is there a way to tell the library to only run it once for some random seed?

Probably, but here I want it to run with multiple random seeds.

jsancio

LGTM.

hachikuji · 2021-03-18T01:12:47Z

I am planning to merge this shortly if there are no objections. I double-checked with @junrao about the note added to NOTICE in regard to the jqwik license.

Conflicts: * build.gradle: keep `dependencySubstitution` Confluent addition in `resolutionStrategy` and take upstream changes. Commits: * apache-github/trunk: KAFKA-12503: inform threads to resize their cache instead of doing so for them (apache#10356) KAFKA-10697: Remove ProduceResponse.responses (apache#10332) MINOR: Exclude KIP-500.md from rat check (apache#10354) MINOR: Move `configurations.all` to be a child of `allprojects` (apache#10349) MINOR: Remove use of `NoSuchElementException` in `KafkaMetadataLog` (apache#10344) MINOR: Start the broker-to-controller channel for request forwarding (apache#10340) KAFKA-12382: add a README for KIP-500 (apache#10227) MINOR: Fix BaseHashTable sizing (apache#10334) KAFKA-10357: Add setup method to internal topics (apache#10317) MINOR: remove redundant null check when testing specified type (apache#10314) KAFKA-12293: Remove JCenter from buildscript and delete buildscript.gradle KAFKA-12491: Make rocksdb an `api` dependency for `streams` (apache#10341) KAFKA-12454: Add ERROR logging on kafka-log-dirs when given brokerIds do not exist in current kafka cluster (apache#10304) KAFKA-12459; Use property testing library for raft event simulation tests (apache#10323) MINOR: fix failing ZooKeeper system tests (apache#10297) MINOR: fix client_compatibility_features_test.py (apache#10292)

hachikuji force-pushed the KAFKA-12459-improve-raft-sim-tests branch from 96cb294 to 35ab2c9 Compare March 15, 2021 23:11

KAFKA-12459; Use property testing library for raft event simulation t…

ec936bb

…ests

hachikuji force-pushed the KAFKA-12459-improve-raft-sim-tests branch from 35ab2c9 to ec936bb Compare March 15, 2021 23:21

dajac approved these changes Mar 16, 2021

View reviewed changes

raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java Outdated Show resolved Hide resolved

chia7712 approved these changes Mar 16, 2021

View reviewed changes

Review comment and small cleanup

587be91

Add mention of jqwik EPL2 in NOTICE file

43035c7

hachikuji force-pushed the KAFKA-12459-improve-raft-sim-tests branch from 0501c45 to 43035c7 Compare March 17, 2021 16:31

jsancio reviewed Mar 17, 2021

View reviewed changes

Improve performance of simulation tests

fab16de

jsancio approved these changes Mar 17, 2021

View reviewed changes

jsancio mentioned this pull request Mar 17, 2021

MINOR: Remove use of NoSuchElementException #10344

Merged

3 tasks

hachikuji merged commit 8ef1619 into apache:trunk Mar 18, 2021

ijuma added the kraft label Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-12459; Use property testing library for raft event simulation tests #10323

KAFKA-12459; Use property testing library for raft event simulation tests #10323

hachikuji commented Mar 15, 2021

dajac left a comment

chia7712 left a comment

chia7712 Mar 16, 2021

ijuma commented Mar 16, 2021

hachikuji commented Mar 16, 2021

ijuma commented Mar 16, 2021

stanislavkozlovski commented Mar 16, 2021

ijuma commented Mar 16, 2021 •

edited

Loading

hachikuji commented Mar 17, 2021

chia7712 commented Mar 17, 2021

hachikuji commented Mar 17, 2021

jsancio left a comment

jsancio Mar 17, 2021

hachikuji Mar 17, 2021

hachikuji Mar 17, 2021

jsancio Mar 17, 2021

jsancio Mar 17, 2021

hachikuji Mar 17, 2021 •

edited

Loading

jsancio left a comment

hachikuji commented Mar 18, 2021 •

edited

Loading

KAFKA-12459; Use property testing library for raft event simulation tests #10323

KAFKA-12459; Use property testing library for raft event simulation tests #10323

Conversation

hachikuji commented Mar 15, 2021

Committer Checklist (excluded from commit message)

dajac left a comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

chia7712 Mar 16, 2021

Choose a reason for hiding this comment

ijuma commented Mar 16, 2021

hachikuji commented Mar 16, 2021

ijuma commented Mar 16, 2021

stanislavkozlovski commented Mar 16, 2021

Quickcheck

Jqwik

ijuma commented Mar 16, 2021 • edited Loading

hachikuji commented Mar 17, 2021

chia7712 commented Mar 17, 2021

hachikuji commented Mar 17, 2021

jsancio left a comment

Choose a reason for hiding this comment

jsancio Mar 17, 2021

Choose a reason for hiding this comment

hachikuji Mar 17, 2021

Choose a reason for hiding this comment

hachikuji Mar 17, 2021

Choose a reason for hiding this comment

jsancio Mar 17, 2021

Choose a reason for hiding this comment

jsancio Mar 17, 2021

Choose a reason for hiding this comment

hachikuji Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

jsancio left a comment

Choose a reason for hiding this comment

hachikuji commented Mar 18, 2021 • edited Loading

ijuma commented Mar 16, 2021 •

edited

Loading

hachikuji Mar 17, 2021 •

edited

Loading

hachikuji commented Mar 18, 2021 •

edited

Loading