facebook / rocksdb

A library that provides an embeddable, persistent key-value store for fast storage.
http://rocksdb.org
GNU General Public License v2.0
28.75k stars 6.34k forks source link

Flaky test DynamicUniversalCompactionReadAmplification #5409

Open rbrasga opened 5 years ago

rbrasga commented 5 years ago

In reference to the test case DynamicUniversalCompactionReadAmplification at: db/db_universal_compaction_test.cc : 454 to 558

This test case failure was mentioned here (as a result of different changes) and apparently never resolved: https://github.com/facebook/rocksdb/pull/4562

Expected behavior

I expect make commit_prereq to run successfully to completion

The comment in the code states: "Flush whatever is remaining in memtable. This is typically small, about 30KB."

Actual behavior

"whatever is remaining in memtable" ends up being ~ 90KB (not the 30KB explained in the comment).

The files being written out of memtables have only 8 key/value pairs (as opposed to the expected 10). I'm not sure why the memtables are closer to 80KB in size rather than 100KB because: options.write_buffer_size = 100 << 10; // 100KB

These test cases are failing.

[ RUN ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/0 db/db_universal_compaction_test.cc:507: Failure Value of: num + 1 Actual: 3 Expected: NumSortedRuns(1) Which is: 4 [ FAILED ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/0, where GetParam() = (1, false) (209 ms)

These test cases fail the same way: [ FAILED ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/0, where GetParam() = (1, false) [ FAILED ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/1, where GetParam() = (1, true) [ FAILED ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/2, where GetParam() = (3, false) [ FAILED ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/3, where GetParam() = (3, true) [ FAILED ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/4, where GetParam() = (5, false) [ FAILED ] UniversalCompactionNumLevels/DBTestUniversalCompaction.DynamicUniversalCompactionReadAmplification/5, where GetParam() = (5, true)

Steps to reproduce the behavior

Run make commit_prereq or make dbg && ./db_universal_compaction_test

Notes:

Upon further investigation, the "expected" numbers appear to be wrong (and the actual numbers seem to be correct)

Here's the diff that resolves the issue:

dbfull()->TEST_WaitForFlushMemTable(handles_[1]); - ASSERT_EQ(NumSortedRuns(1), num + 1); + if (num < 2){ASSERT_EQ(NumSortedRuns(1), num + 1);} + else{ASSERT_EQ(NumSortedRuns(1), num + 2);} } - ASSERT_EQ(NumSortedRuns(1), options.level0_file_num_compaction_trigger); + ASSERT_EQ(NumSortedRuns(1), options.level0_file_num_compaction_trigger + 1);

// Flush whatever is remaining in memtable. This is typically small, about // 30KB. ASSERT_OK(Flush(1)); dbfull()->TEST_WaitForCompact(); // Verify compaction did not happen - ASSERT_EQ(NumSortedRuns(1), options.level0_file_num_compaction_trigger + 1); + ASSERT_EQ(NumSortedRuns(1), options.level0_file_num_compaction_trigger + 2); ASSERT_EQ(total_picked_compactions, 0);

ASSERT_OK(dbfull()->SetOptions( @@ -552,8 +553,8 @@ TEST_P(DBTestUniversalCompaction, DynamicUniversalCompactionReadAmplification) { // If max_merge_width had not been changed dynamically above, and if it // continued to be the default value of UINIT_MAX, total_picked_compactions // would have been 1. - ASSERT_EQ(total_picked_compactions, 2); - ASSERT_EQ(total_size_ratio_compactions, 2); + ASSERT_EQ(total_picked_compactions, 3); + ASSERT_EQ(total_size_ratio_compactions, 3);

rbrasga commented 5 years ago

Also, make this change on line 501 resolved the issue:

-for (int i = 0; i < 11; i++) { +for (int i = 0; i < 9; i++) {

rbrasga commented 5 years ago

A different potential fix is:

db/db_universal_compaction_test.cc:469 - options.write_buffer_size = 100 << 10; // 100KB + options.write_buffer_size = 120 << 10; // 120KB

rbrasga commented 5 years ago

Note: those changes work for the default unit tests, but fail for ASAN.

rbrasga commented 5 years ago

It looks like my issue was ROCKSDB_JEMALLOC is enabled during the normal 'unit' test run, and disabled during the 'asan' test run.

+#ifdef ROCKSDB_JEMALLOC + options.write_buffer_size = 120 << 10; // 120KB (75% buffer) +#else options.write_buffer_size = 100 << 10; // 100KB +#endif

maysamyabandeh commented 5 years ago

@rbrasga The test seems to be badly designed and present different behaviors in different platforms. The proper fix seems to be a redesign of the test rather than tweaking the numbers. Let's keep this issue open in case one was interested in working on it.