Fix some unit test failure in ExternalSSTFileBasicTest#11070
Closed
cbi42 wants to merge 1 commit intofacebook:mainfrom
Closed
Fix some unit test failure in ExternalSSTFileBasicTest#11070cbi42 wants to merge 1 commit intofacebook:mainfrom
cbi42 wants to merge 1 commit intofacebook:mainfrom
Conversation
Contributor
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
approved these changes
Jan 5, 2023
Contributor
There was a problem hiding this comment.
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.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary: valgrind build for
ExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithMixedValueTypeandExternalSSTFileBasicTest/ExternalSSTFileBasicTest.IngestFileWithGlobalSeqnoPickedSeqnostarted 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.