crash_test to do some verification for prefix extractor and iterator bounds. #5846

siying · 2019-09-23T22:30:25Z

Summary:
For now, crash_test is not able to report any failure for the logic related to iterator upper, lower bounds or iterators, or reseek. These are features prone to errors. Improve db_stress in several ways:
(1) For each iterator run, reseek up to 3 times.
(2) For every iterator, create control iterator with upper or lower bound, with total order seek. Compare the results with the iterator.
(3) Make simple crash test to avoid prefix size to have more coverage.
(4) make prefix_size = 0 a valid size and -1 to indicate disabling prefix extractor.

Test Plan: Manually hack the code to create wrong results and see they are caught by the tool.

Summary: For now, crash_test is not able to report any failure for the logic related to iterator upper, lower bounds or iterators, or reseek. These are features prone to errors. Improve db_stress in several ways: (1) For each iterator run, reseek up to 3 times. (2) For every iterator, create control iterator with upper or lower bound, with total order seek. Compare the results with the iterator. (3) Make simple crash test to avoid prefix size to have more coverage. Test Plan: Manually hack the code to create wrong results and see they are caught by the tool.

siying · 2019-09-24T00:26:06Z

I don't know it's a good thing or bad thing but, after running crash test for several hours with the PR, the assertion failed.

siying · 2019-09-24T20:38:49Z

I relaxed the verification condition in the prefix case. I'm rerunning the test and see whether it still fails.

maysamyabandeh · 2019-09-25T15:56:47Z

tools/db_stress.cc

+                     FLAGS_ops_per_thread - i - 1));
+        rand_keys = GenerateNKeys(thread, num_seeks, i);
+        TestMultiGet(thread, read_opts, rand_column_families, rand_keys);
+        i += num_seeks - 1;


Can you add inline comments of why we are doing reseek (via MultiGet I guess?) before iterate?

Hmm.. It looks like a bug that I copied TestMultiGet() by mistake... Let me fix it.

maysamyabandeh · 2019-09-25T16:06:14Z

tools/db_stress.cc

+              iter->key().ToString(true).c_str(),
+              seek_key.ToString(true).c_str());
+
+      *diverged = true;


We set diverged to true but we do not set any error here!

The error is set in line 2616.

maysamyabandeh · 2019-09-25T16:07:33Z

tools/db_stress.cc

+      for (uint64_t i = 0; i < FLAGS_num_iterations && iter->Valid(); i++) {
+        if (thread->rand.OneIn(2)) {
+          iter->Next();
+          if (cmp_iter->Valid()) {


The if statement could be removed if add diverged to the for loop condition.

But what's the point of complicating the code further more while the performance here is not critical.

I found that simpler. But this is not a big deal.

I can do that.

I meant "diverged to the for loop condition." not to the "if condition". But it does not matter much.

It's still valuable to do Next(), Prev() after diverged to make sure it doesn't crash, not?

maysamyabandeh · 2019-09-25T16:08:54Z

tools/db_stress.cc

@@ -3814,8 +3954,8 @@ class BatchedOpsStressTest : public StressTest {
        fprintf(stderr, "error : inconsistent values for key %s: %s, %s\n",
                key.ToString(true).c_str(), StringToHex(values[0]).c_str(),
                StringToHex(values[i]).c_str());
-      // we continue after error rather than exiting so that we can
-      // find more errors if any
+        // we continue after error rather than OAexiting so that we can


Any comment on this?

Sorry I overlooked it. Will fix.

maysamyabandeh · 2019-09-25T16:28:14Z

tools/db_crashtest.py

@@ -124,8 +124,9 @@ def is_direct_io_supported(dbname):
    "max_background_compactions": 1,
    "max_bytes_for_level_base": 67108864,
    "memtablerep": "skip_list",
-    "prefixpercent": 25,
-    "readpercent": 25,
+    "prefixpercent": 0,


Why prefix is disabled?

I just want to have some test to cover non-prefix case.

I am confused. So the tests that covered prefix cases are gone now?

This is in simple_default_params structure. So the change disable prefix test in simple crash test. We run simple, normal and atomic flush crash test, so prefix tests are still there in 2/3 of the tests.

Can you update this sentence in the PR description? "Make simple crash test to avoid prefix size to have more coverage." If i understood it right, you meant to say: "Limit prefix tests to only normal and atomic flush crash test, to let simple crash tests have higher coverage on non-prefix workloads."

maysamyabandeh · 2019-09-25T16:30:00Z

tools/db_crashtest.py

-    "readpercent": 25,
+    "prefixpercent": 0,
+    "readpercent": 50,
+    "prefix_size" : -1,


Why do we need two parameters to enable/disable prefix tests?

prefixpercent is to determine whether to run the queries about prefix scan, and "prefix_size" is to determine prefix_extractor to use. Unless we disable prefix extractor, other tests will still go through it.

siying · 2019-09-26T23:06:56Z

@maysamyabandeh do you think all comments are properly addressed?

facebook-github-bot

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-09-27T18:39:51Z

This pull request has been merged in 679a45d.

…bounds. (facebook#5846) Summary: For now, crash_test is not able to report any failure for the logic related to iterator upper, lower bounds or iterators, or reseek. These are features prone to errors. Improve db_stress in several ways: (1) For each iterator run, reseek up to 3 times. (2) For every iterator, create control iterator with upper or lower bound, with total order seek. Compare the results with the iterator. (3) Make simple crash test to avoid prefix size to have more coverage. (4) make prefix_size = 0 a valid size and -1 to indicate disabling prefix extractor. Pull Request resolved: facebook#5846 Test Plan: Manually hack the code to create wrong results and see they are caught by the tool. Differential Revision: D17631760 fbshipit-source-id: acd460a177bd2124a5ffd7fff490702dba63030b

siying requested a review from maysamyabandeh September 23, 2019 22:30

facebook-github-bot added the CLA Signed label Sep 23, 2019

siying added 3 commits September 23, 2019 18:16

Improve error message

c48aed8

Stop verifying after out of prefix.

d9c9d3d

Make prefix_size = 0 a valid prefix length.

a120a47

maysamyabandeh reviewed Sep 25, 2019

View reviewed changes

siying added 2 commits September 25, 2019 16:45

Remove the extra TestMultiGet()

a1b8447

Address comment

963f06e

maysamyabandeh approved these changes Sep 27, 2019

View reviewed changes

siying changed the title ~~crash_test to do some verify for prefix extractor and iterator bounds.~~ crash_test to do some verification for prefix extractor and iterator bounds. Sep 27, 2019

facebook-github-bot reviewed Sep 27, 2019

View reviewed changes

facebook-github-bot closed this in 679a45d Sep 27, 2019

facebook-github-bot added the Merged label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crash_test to do some verification for prefix extractor and iterator bounds. #5846

crash_test to do some verification for prefix extractor and iterator bounds. #5846

siying commented Sep 23, 2019 •

edited

Loading

siying commented Sep 24, 2019

siying commented Sep 24, 2019

maysamyabandeh Sep 25, 2019

siying Sep 25, 2019

maysamyabandeh Sep 25, 2019

siying Sep 25, 2019

maysamyabandeh Sep 25, 2019

siying Sep 25, 2019

maysamyabandeh Sep 26, 2019

siying Sep 26, 2019

maysamyabandeh Sep 27, 2019

siying Sep 27, 2019

maysamyabandeh Sep 25, 2019

maysamyabandeh Sep 26, 2019

siying Sep 26, 2019

maysamyabandeh Sep 25, 2019

siying Sep 25, 2019

maysamyabandeh Sep 26, 2019

siying Sep 26, 2019

maysamyabandeh Sep 27, 2019

maysamyabandeh Sep 25, 2019

siying Sep 25, 2019

siying commented Sep 26, 2019

facebook-github-bot left a comment

facebook-github-bot commented Sep 27, 2019

crash_test to do some verification for prefix extractor and iterator bounds. #5846

crash_test to do some verification for prefix extractor and iterator bounds. #5846

Conversation

siying commented Sep 23, 2019 • edited Loading

siying commented Sep 24, 2019

siying commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siying commented Sep 26, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 27, 2019

siying commented Sep 23, 2019 •

edited

Loading