From 3073b1c573fac350912bb59b874ab2fe210a1dad Mon Sep 17 00:00:00 2001 From: Maysam Yabandeh Date: Mon, 29 Jan 2018 17:03:23 -0800 Subject: [PATCH] Split SnapshotConcurrentAccessTest into 20 sub tests Summary: SnapshotConcurrentAccessTest sometimes times out when running on the test infra. This patch splits the test into smaller sub-tests to avoid the timeout. It also benefits from lower run-time of each sub-test and increases the coverage of the test. The overall run-time of each final sub-test is at most half of the original test so we should no longer see a timeout. Closes https://github.com/facebook/rocksdb/pull/3435 Differential Revision: D6839427 Pulled By: maysamyabandeh fbshipit-source-id: d53fdb157109e2438ca7fe447d0cf4b71f304bd8 --- utilities/transactions/transaction_test.h | 29 +++-- .../write_prepared_transaction_test.cc | 102 ++++++++++++++++-- .../transactions/write_prepared_txn_db.h | 4 +- 3 files changed, 113 insertions(+), 22 deletions(-) diff --git a/utilities/transactions/transaction_test.h b/utilities/transactions/transaction_test.h index 61d5d1f63..522f95dfd 100644 --- a/utilities/transactions/transaction_test.h +++ b/utilities/transactions/transaction_test.h @@ -36,8 +36,7 @@ namespace rocksdb { -class TransactionTest : public ::testing::TestWithParam< - std::tuple> { +class TransactionTestBase : public ::testing::Test { public: TransactionDB* db; FaultInjectionTestEnv* env; @@ -45,8 +44,11 @@ class TransactionTest : public ::testing::TestWithParam< Options options; TransactionDBOptions txn_db_options; + bool use_stackable_db_; - TransactionTest() { + TransactionTestBase(bool use_stackable_db, bool two_write_queue, + TxnDBWritePolicy write_policy) + : use_stackable_db_(use_stackable_db) { options.create_if_missing = true; options.max_write_buffer_number = 2; options.write_buffer_size = 4 * 1024; @@ -54,15 +56,15 @@ class TransactionTest : public ::testing::TestWithParam< options.merge_operator = MergeOperators::CreateFromStringId("stringappend"); env = new FaultInjectionTestEnv(Env::Default()); options.env = env; - options.two_write_queues = std::get<1>(GetParam()); + options.two_write_queues = two_write_queue; dbname = test::TmpDir() + "/transaction_testdb"; DestroyDB(dbname, options); txn_db_options.transaction_lock_timeout = 0; txn_db_options.default_lock_timeout = 0; - txn_db_options.write_policy = std::get<2>(GetParam()); + txn_db_options.write_policy = write_policy; Status s; - if (std::get<0>(GetParam()) == false) { + if (use_stackable_db == false) { s = TransactionDB::Open(options, txn_db_options, dbname, &db); } else { s = OpenWithStackableDB(); @@ -70,7 +72,7 @@ class TransactionTest : public ::testing::TestWithParam< assert(s.ok()); } - ~TransactionTest() { + ~TransactionTestBase() { delete db; // This is to skip the assert statement in FaultInjectionTestEnv. There // seems to be a bug in btrfs that the makes readdir return recently @@ -88,7 +90,7 @@ class TransactionTest : public ::testing::TestWithParam< env->DropUnsyncedFileData(); env->ResetState(); Status s; - if (std::get<0>(GetParam()) == false) { + if (use_stackable_db_ == false) { s = TransactionDB::Open(options, txn_db_options, dbname, &db); } else { s = OpenWithStackableDB(); @@ -100,7 +102,7 @@ class TransactionTest : public ::testing::TestWithParam< delete db; DestroyDB(dbname, options); Status s; - if (std::get<0>(GetParam()) == false) { + if (use_stackable_db_ == false) { s = TransactionDB::Open(options, txn_db_options, dbname, &db); } else { s = OpenWithStackableDB(); @@ -397,6 +399,15 @@ class TransactionTest : public ::testing::TestWithParam< } }; +class TransactionTest : public TransactionTestBase, + public ::testing::WithParamInterface< + std::tuple> { + public: + TransactionTest() + : TransactionTestBase(std::get<0>(GetParam()), std::get<1>(GetParam()), + std::get<2>(GetParam())){}; +}; + class MySQLStyleTransactionTest : public TransactionTest {}; } // namespace rocksdb diff --git a/utilities/transactions/write_prepared_transaction_test.cc b/utilities/transactions/write_prepared_transaction_test.cc index b41b076dc..5e8a88059 100644 --- a/utilities/transactions/write_prepared_transaction_test.cc +++ b/utilities/transactions/write_prepared_transaction_test.cc @@ -181,7 +181,12 @@ class WritePreparedTxnDBMock : public WritePreparedTxnDB { std::vector snapshots_; }; -class WritePreparedTransactionTest : public TransactionTest { +class WritePreparedTransactionTestBase : public TransactionTestBase { + public: + WritePreparedTransactionTestBase(bool use_stackable_db, bool two_write_queue, + TxnDBWritePolicy write_policy) + : TransactionTestBase(use_stackable_db, two_write_queue, write_policy){}; + protected: // If expect_update is set, check if it actually updated old_commit_map_. If // it did not and yet suggested not to check the next snapshot, do the @@ -342,11 +347,87 @@ class WritePreparedTransactionTest : public TransactionTest { } }; +class WritePreparedTransactionTest + : public WritePreparedTransactionTestBase, + public ::testing::WithParamInterface< + std::tuple> { + public: + WritePreparedTransactionTest() + : WritePreparedTransactionTestBase(std::get<0>(GetParam()), + std::get<1>(GetParam()), + std::get<2>(GetParam())){}; +}; + +class SnapshotConcurrentAccessTest + : public WritePreparedTransactionTestBase, + public ::testing::WithParamInterface< + std::tuple> { + public: + SnapshotConcurrentAccessTest() + : WritePreparedTransactionTestBase(std::get<0>(GetParam()), + std::get<1>(GetParam()), + std::get<2>(GetParam())), + split_id_(std::get<3>(GetParam())), + split_cnt_(std::get<4>(GetParam())){}; + + protected: + // A test is split into split_cnt_ tests, each identified with split_id_ where + // 0 <= split_id_ < split_cnt_ + size_t split_id_; + size_t split_cnt_; +}; + INSTANTIATE_TEST_CASE_P( WritePreparedTransactionTest, WritePreparedTransactionTest, ::testing::Values(std::make_tuple(false, false, WRITE_PREPARED), std::make_tuple(false, true, WRITE_PREPARED))); +INSTANTIATE_TEST_CASE_P( + TwoWriteQueues, SnapshotConcurrentAccessTest, + ::testing::Values(std::make_tuple(false, true, WRITE_PREPARED, 0, 20), + std::make_tuple(false, true, WRITE_PREPARED, 1, 20), + std::make_tuple(false, true, WRITE_PREPARED, 2, 20), + std::make_tuple(false, true, WRITE_PREPARED, 3, 20), + std::make_tuple(false, true, WRITE_PREPARED, 4, 20), + std::make_tuple(false, true, WRITE_PREPARED, 5, 20), + std::make_tuple(false, true, WRITE_PREPARED, 6, 20), + std::make_tuple(false, true, WRITE_PREPARED, 7, 20), + std::make_tuple(false, true, WRITE_PREPARED, 8, 20), + std::make_tuple(false, true, WRITE_PREPARED, 9, 20), + std::make_tuple(false, true, WRITE_PREPARED, 10, 20), + std::make_tuple(false, true, WRITE_PREPARED, 11, 20), + std::make_tuple(false, true, WRITE_PREPARED, 12, 20), + std::make_tuple(false, true, WRITE_PREPARED, 13, 20), + std::make_tuple(false, true, WRITE_PREPARED, 14, 20), + std::make_tuple(false, true, WRITE_PREPARED, 15, 20), + std::make_tuple(false, true, WRITE_PREPARED, 16, 20), + std::make_tuple(false, true, WRITE_PREPARED, 17, 20), + std::make_tuple(false, true, WRITE_PREPARED, 18, 20), + std::make_tuple(false, true, WRITE_PREPARED, 19, 20))); + +INSTANTIATE_TEST_CASE_P( + OneWriteQueue, SnapshotConcurrentAccessTest, + ::testing::Values(std::make_tuple(false, false, WRITE_PREPARED, 0, 20), + std::make_tuple(false, false, WRITE_PREPARED, 1, 20), + std::make_tuple(false, false, WRITE_PREPARED, 2, 20), + std::make_tuple(false, false, WRITE_PREPARED, 3, 20), + std::make_tuple(false, false, WRITE_PREPARED, 4, 20), + std::make_tuple(false, false, WRITE_PREPARED, 5, 20), + std::make_tuple(false, false, WRITE_PREPARED, 6, 20), + std::make_tuple(false, false, WRITE_PREPARED, 7, 20), + std::make_tuple(false, false, WRITE_PREPARED, 8, 20), + std::make_tuple(false, false, WRITE_PREPARED, 9, 20), + std::make_tuple(false, false, WRITE_PREPARED, 10, 20), + std::make_tuple(false, false, WRITE_PREPARED, 11, 20), + std::make_tuple(false, false, WRITE_PREPARED, 12, 20), + std::make_tuple(false, false, WRITE_PREPARED, 13, 20), + std::make_tuple(false, false, WRITE_PREPARED, 14, 20), + std::make_tuple(false, false, WRITE_PREPARED, 15, 20), + std::make_tuple(false, false, WRITE_PREPARED, 16, 20), + std::make_tuple(false, false, WRITE_PREPARED, 17, 20), + std::make_tuple(false, false, WRITE_PREPARED, 18, 20), + std::make_tuple(false, false, WRITE_PREPARED, 19, 20))); + TEST_P(WritePreparedTransactionTest, CommitMapTest) { WritePreparedTxnDB* wp_db = dynamic_cast(db); assert(wp_db); @@ -553,26 +634,24 @@ bool IsInCombination(size_t i, size_t comb) { return comb & (size_t(1) << i); } #ifndef TRAVIS // Test that CheckAgainstSnapshots will not miss a live snapshot if it is run in // parallel with UpdateSnapshots. -TEST_P(WritePreparedTransactionTest, SnapshotConcurrentAccessTest) { +TEST_P(SnapshotConcurrentAccessTest, SnapshotConcurrentAccessTest) { // We have a sync point in the method under test after checking each snapshot. // If you increase the max number of snapshots in this test, more sync points // in the methods must also be added. - const std::vector snapshots = {10l, 20l, 30l, 40l, 50l, 60l}; - // TODO(myabandeh): increase the snapshots list for pre-release tests - // const std::vector snapshots = {10l, 20l, 30l, 40l, 50l, - // 60l, 70l, 80l, 90l, 100l}; + const std::vector snapshots = {10l, 20l, 30l, 40l, 50l, + 60l, 70l, 80l, 90l, 100l}; const size_t snapshot_cache_bits = 2; // Safety check to express the intended size in the test. Can be adjusted if // the snapshots lists changed. - assert((1ul << snapshot_cache_bits) + 2 == snapshots.size()); + assert((1ul << snapshot_cache_bits) * 2 + 2 == snapshots.size()); SequenceNumber version = 1000l; // Choose the cache size so that the new snapshot list could replace all the // existing items in the cache and also have some overflow. DBImpl* mock_db = new DBImpl(options, dbname); std::unique_ptr wp_db( new WritePreparedTxnDBMock(mock_db, txn_db_options, snapshot_cache_bits)); - // TODO(myabandeh): increase this number for pre-release tests - const size_t extra = 1; + const size_t extra = 2; + size_t loop_id = 0; // Add up to extra items that do not fit into the cache for (size_t old_size = 1; old_size <= wp_db->SNAPSHOT_CACHE_SIZE + extra; old_size++) { @@ -582,7 +661,8 @@ TEST_P(WritePreparedTransactionTest, SnapshotConcurrentAccessTest) { // Each member of old snapshot might or might not appear in the new list. We // create a common_snapshots for each combination. size_t new_comb_cnt = size_t(1) << old_size; - for (size_t new_comb = 0; new_comb < new_comb_cnt; new_comb++) { + for (size_t new_comb = 0; new_comb < new_comb_cnt; new_comb++, loop_id++) { + if (loop_id % split_cnt_ != split_id_) continue; printf("."); // To signal progress fflush(stdout); std::vector common_snapshots; @@ -630,7 +710,7 @@ TEST_P(WritePreparedTransactionTest, SnapshotConcurrentAccessTest) { } printf("\n"); } -#endif +#endif // TRAVIS // This test clarifies the contract of AdvanceMaxEvictedSeq method TEST_P(WritePreparedTransactionTest, AdvanceMaxEvictedSeqBasicTest) { diff --git a/utilities/transactions/write_prepared_txn_db.h b/utilities/transactions/write_prepared_txn_db.h index fd92a04e2..4e83609fe 100644 --- a/utilities/transactions/write_prepared_txn_db.h +++ b/utilities/transactions/write_prepared_txn_db.h @@ -209,8 +209,8 @@ class WritePreparedTxnDB : public PessimisticTransactionDB { friend class WritePreparedTransactionTest_IsInSnapshotTest_Test; friend class WritePreparedTransactionTest_CheckAgainstSnapshotsTest_Test; friend class WritePreparedTransactionTest_CommitMapTest_Test; - friend class WritePreparedTransactionTest_SnapshotConcurrentAccessTest_Test; - friend class WritePreparedTransactionTest; + friend class SnapshotConcurrentAccessTest_SnapshotConcurrentAccessTest_Test; + friend class WritePreparedTransactionTestBase; friend class PreparedHeap_BasicsTest_Test; friend class WritePreparedTxnDBMock; friend class WritePreparedTransactionTest_AdvanceMaxEvictedSeqBasicTest_Test;