Skip to content

Fix some unit test failure in ExternalSSTFileBasicTest#11070

Closed
cbi42 wants to merge 1 commit intofacebook:mainfrom
cbi42:fix-external-ingest-valgrind
Closed

Fix some unit test failure in ExternalSSTFileBasicTest#11070
cbi42 wants to merge 1 commit intofacebook:mainfrom
cbi42:fix-external-ingest-valgrind

Conversation

@cbi42
Copy link
Copy Markdown
Contributor

@cbi42 cbi42 commented Jan 3, 2023

Summary: valgrind build for ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithMixedValueType and ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithGlobalSeqnoPickedSeqno started failing (see error message in T141554665). I could not repro but I suspect it is due to file ingestion range overlapping with ongoing compaction, which caused a new global seqno being assigned after #10988.

Test plan: monitor future valgrind tests result.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cbi42 cbi42 requested a review from hx235 January 3, 2023 19:40
Copy link
Copy Markdown
Contributor

@hx235 hx235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I had to be pretty intentional in reproing this one but your theory is correct. See below for the repro.

diff --git a/db/external_sst_file_basic_test.cc b/db/external_sst_file_basic_test.cc
index 97b989733..c4106aa28 100644
--- a/db/external_sst_file_basic_test.cc
+++ b/db/external_sst_file_basic_test.cc
@@ -693,11 +693,20 @@ TEST_P(ExternalSSTFileBasicTest, IngestFileWithGlobalSeqnoPickedSeqno) {
   bool write_global_seqno = std::get<0>(GetParam());
   bool verify_checksums_before_ingest = std::get<1>(GetParam());
   do {
+    if(option_config_ != kUniversalCompaction){
+      continue;
+    }
     Options options = CurrentOptions();
     DestroyAndReopen(options);
     std::map<std::string, std::string> true_data;
 
     int file_id = 1;
+        std::shared_ptr<test::SleepingBackgroundTask> sleeping_task_;
+    sleeping_task_.reset(new test::SleepingBackgroundTask());
+    env_->SetBackgroundThreads(1, Env::LOW);
+    env_->Schedule(&test::SleepingBackgroundTask::DoSleepTask,
+                   sleeping_task_.get(), Env::Priority::LOW);
+    sleeping_task_->WaitUntilSleeping();
 
     ASSERT_OK(GenerateAndAddExternalFile(
         options, {1, 2, 3, 4, 5, 6}, ValueType::kTypeValue, file_id++,
@@ -742,6 +751,17 @@ TEST_P(ExternalSSTFileBasicTest, IngestFileWithGlobalSeqnoPickedSeqno) {
     }
     SequenceNumber last_seqno = dbfull()->GetLatestSequenceNumber();
 
+    SyncPoint::GetInstance()->LoadDependency({
+      {"DBImpl::BackgroundCompaction:NonTrivial:BeforeRun", "pre_file_ingestion"},
+      {"ExternalSstFileIngestionJob::Run",
+       "DBImpl::BackgroundCompaction:NonTrivial:AfterRun"},
+    });
+     SyncPoint::GetInstance()->EnableProcessing();
+    sleeping_task_->WakeUp();
+    sleeping_task_->WaitUntilDone();
+
+    TEST_SYNC_POINT("pre_file_ingestion");
+
     ASSERT_OK(GenerateAndAddExternalFile(
         options, {60, 61, 62}, ValueType::kTypeValue, file_id++,
         write_global_seqno, verify_checksums_before_ingest, &true_data));
@@ -927,12 +947,21 @@ TEST_P(ExternalSSTFileBasicTest, IngestFileWithMixedValueType) {
   bool write_global_seqno = std::get<0>(GetParam());
   bool verify_checksums_before_ingest = std::get<1>(GetParam());
   do {
+    if(option_config_ != kUniversalCompaction){
+      continue;
+    }
     Options options = CurrentOptions();
     options.merge_operator.reset(new TestPutOperator());
     DestroyAndReopen(options);
     std::map<std::string, std::string> true_data;
 
     int file_id = 1;
+    std::shared_ptr<test::SleepingBackgroundTask> sleeping_task_;
+    sleeping_task_.reset(new test::SleepingBackgroundTask());
+    env_->SetBackgroundThreads(1, Env::LOW);
+    env_->Schedule(&test::SleepingBackgroundTask::DoSleepTask,
+                   sleeping_task_.get(), Env::Priority::LOW);
+    sleeping_task_->WaitUntilSleeping();
 
     ASSERT_OK(GenerateAndAddExternalFile(
         options, {1, 2, 3, 4, 5, 6},
@@ -1017,6 +1046,16 @@ TEST_P(ExternalSSTFileBasicTest, IngestFileWithMixedValueType) {
     }
     SequenceNumber last_seqno = dbfull()->GetLatestSequenceNumber();
 
+    SyncPoint::GetInstance()->LoadDependency({
+      {"DBImpl::BackgroundCompaction:NonTrivial:BeforeRun", "pre_file_ingestion"},
+      {"ExternalSstFileIngestionJob::Run",
+       "DBImpl::BackgroundCompaction:NonTrivial:AfterRun"},
+    });
+     SyncPoint::GetInstance()->EnableProcessing();
+    sleeping_task_->WakeUp();
+    sleeping_task_->WaitUntilDone();
+
+    TEST_SYNC_POINT("pre_file_ingestion");
     ASSERT_OK(GenerateAndAddExternalFile(
         options, {60, 61, 62},
         {ValueType::kTypeValue, ValueType::kTypeMerge, ValueType::kTypeValue},

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExternalSSTFileBasicTest/ExternalSSTFileBasicTest
[ RUN      ] ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithMixedValueType/1
db/external_sst_file_basic_test.cc:1069: Failure
Expected equality of these values:
  dbfull()->GetLatestSequenceNumber()
    Which is: 56
  last_seqno
    Which is: 55
[  FAILED  ] ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithMixedValueType/1, where GetParam() = (true, false) (4888 ms)
[----------] 1 test from ExternalSSTFileBasicTest/ExternalSSTFileBasicTest (4888 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (4888 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithMixedValueType/1, where GetParam() = (true, false)

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExternalSSTFileBasicTest/ExternalSSTFileBasicTest
[ RUN      ] ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithGlobalSeqnoPickedSeqno/1
db/external_sst_file_basic_test.cc:771: Failure
Expected equality of these values:
  dbfull()->GetLatestSequenceNumber()
    Which is: 54
  last_seqno
    Which is: 53
[  FAILED  ] ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithGlobalSeqnoPickedSeqno/1, where GetParam() = (true, false) (16725 ms)
[----------] 1 test from ExternalSSTFileBasicTest/ExternalSSTFileBasicTest (16725 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (16725 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithGlobalSeqnoPickedSeqno/1, where GetParam() = (true, false)


         
         
         

This difficulties in reproing it might also explain why it never came up in my make check test, despite its similarity with https://github.com/facebook/rocksdb/pull/10988/files#r1032711665

Good news though is I just checked external_sst_file_basic_test and external_sst_file_test and these three seem to be the only affected ones.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@cbi42 merged this pull request in 0a2d3b6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants