Skip to content

Commit c364eb4

Browse files
yuslepukhinfacebook-github-bot
authored andcommitted
Windows cumulative patch
Summary: This patch addressed several issues. Portability including db_test std::thread -> port::Thread Cc: @ and %z to ROCKSDB portable macro. Cc: maysamyabandeh Implement Env::AreFilesSame Make the implementation of file unique number more robust Get rid of C-runtime and go directly to Windows API when dealing with file primitives. Implement GetSectorSize() and aling unbuffered read on the value if available. Adjust Windows Logger for the new interface, implement CloseImpl() Cc: anand1976 Fix test running script issue where $status var was of incorrect scope so the failures were swallowed and not reported. DestroyDB() creates a logger and opens a LOG file in the directory being cleaned up. This holds a lock on the folder and the cleanup is prevented. This fails one of the checkpoin tests. We observe the same in production. We close the log file in this change. Fix DBTest2.ReadAmpBitmapLiveInCacheAfterDBClose failure where the test attempts to open a directory with NewRandomAccessFile which does not work on Windows. Fix DBTest.SoftLimit as it is dependent on thread timing. CC: yiwu-arbug Closes facebook#3552 Differential Revision: D7156304 Pulled By: siying fbshipit-source-id: 43db0a757f1dfceffeb2b7988043156639173f5b
1 parent b864bc9 commit c364eb4

15 files changed

+488
-112
lines changed

build_tools/run_ci_db_test.ps1

+4-4
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ $InvokeTestAsync = {
336336
# Test limiting factor here
337337
[int]$count = 0
338338
# Overall status
339-
[bool]$success = $true;
339+
[bool]$script:success = $true;
340340

341341
function RunJobs($Suites, $TestCmds, [int]$ConcurrencyVal)
342342
{
@@ -425,7 +425,7 @@ function RunJobs($Suites, $TestCmds, [int]$ConcurrencyVal)
425425
$log_content = @(Get-Content $log)
426426

427427
if($completed.State -ne "Completed") {
428-
$success = $false
428+
$script:success = $false
429429
Write-Warning $message
430430
$log_content | Write-Warning
431431
} else {
@@ -449,7 +449,7 @@ function RunJobs($Suites, $TestCmds, [int]$ConcurrencyVal)
449449
}
450450

451451
if(!$pass_found) {
452-
$success = $false;
452+
$script:success = $false;
453453
Write-Warning $message
454454
$log_content | Write-Warning
455455
} else {
@@ -473,7 +473,7 @@ New-TimeSpan -Start $StartDate -End $EndDate |
473473
}
474474

475475

476-
if(!$success) {
476+
if(!$script:success) {
477477
# This does not succeed killing off jobs quick
478478
# So we simply exit
479479
# Remove-Job -Job $jobs -Force

db/db_impl.cc

+4-1
Original file line numberDiff line numberDiff line change
@@ -2389,10 +2389,13 @@ Snapshot::~Snapshot() {
23892389
}
23902390

23912391
Status DestroyDB(const std::string& dbname, const Options& options) {
2392-
const ImmutableDBOptions soptions(SanitizeOptions(dbname, options));
2392+
ImmutableDBOptions soptions(SanitizeOptions(dbname, options));
23932393
Env* env = soptions.env;
23942394
std::vector<std::string> filenames;
23952395

2396+
// Reset the logger because it holds a handle to the
2397+
// log file and prevents cleanup and directory removal
2398+
soptions.info_log.reset();
23962399
// Ignore error in case directory does not exist
23972400
env->GetChildren(dbname, &filenames);
23982401

db/db_test.cc

+32-8
Original file line numberDiff line numberDiff line change
@@ -5153,7 +5153,7 @@ TEST_F(DBTest, AutomaticConflictsWithManualCompaction) {
51535153
}
51545154
ASSERT_OK(Flush());
51555155
}
5156-
std::thread manual_compaction_thread([this]() {
5156+
port::Thread manual_compaction_thread([this]() {
51575157
CompactRangeOptions croptions;
51585158
croptions.exclusive_manual_compaction = true;
51595159
ASSERT_OK(db_->CompactRange(croptions, nullptr, nullptr));
@@ -5393,18 +5393,42 @@ TEST_F(DBTest, HardLimit) {
53935393
#ifndef ROCKSDB_LITE
53945394
class WriteStallListener : public EventListener {
53955395
public:
5396-
WriteStallListener() : condition_(WriteStallCondition::kNormal) {}
5396+
WriteStallListener() : cond_(&mutex_),
5397+
condition_(WriteStallCondition::kNormal),
5398+
expected_(WriteStallCondition::kNormal),
5399+
expected_set_(false)
5400+
{}
53975401
void OnStallConditionsChanged(const WriteStallInfo& info) override {
53985402
MutexLock l(&mutex_);
53995403
condition_ = info.condition.cur;
5404+
if (expected_set_ &&
5405+
condition_ == expected_) {
5406+
cond_.Signal();
5407+
expected_set_ = false;
5408+
}
54005409
}
54015410
bool CheckCondition(WriteStallCondition expected) {
54025411
MutexLock l(&mutex_);
5403-
return expected == condition_;
5412+
if (expected != condition_) {
5413+
expected_ = expected;
5414+
expected_set_ = true;
5415+
while (expected != condition_) {
5416+
// We bail out on timeout 500 milliseconds
5417+
const uint64_t timeout_us = 500000;
5418+
if (cond_.TimedWait(timeout_us)) {
5419+
expected_set_ = false;
5420+
return false;
5421+
}
5422+
}
5423+
}
5424+
return true;
54045425
}
54055426
private:
5406-
port::Mutex mutex_;
5427+
port::Mutex mutex_;
5428+
port::CondVar cond_;
54075429
WriteStallCondition condition_;
5430+
WriteStallCondition expected_;
5431+
bool expected_set_;
54085432
};
54095433

54105434
TEST_F(DBTest, SoftLimit) {
@@ -5743,7 +5767,7 @@ TEST_F(DBTest, ThreadLocalPtrDeadlock) {
57435767
return flushes_done.load() > 10;
57445768
};
57455769

5746-
std::thread flushing_thread([&] {
5770+
port::Thread flushing_thread([&] {
57475771
for (int i = 0; !done(); ++i) {
57485772
ASSERT_OK(db_->Put(WriteOptions(), Slice("hi"),
57495773
Slice(std::to_string(i).c_str())));
@@ -5753,12 +5777,12 @@ TEST_F(DBTest, ThreadLocalPtrDeadlock) {
57535777
}
57545778
});
57555779

5756-
std::vector<std::thread> thread_spawning_threads(10);
5780+
std::vector<port::Thread> thread_spawning_threads(10);
57575781
for (auto& t: thread_spawning_threads) {
5758-
t = std::thread([&] {
5782+
t = port::Thread([&] {
57595783
while (!done()) {
57605784
{
5761-
std::thread tmp_thread([&] {
5785+
port::Thread tmp_thread([&] {
57625786
auto it = db_->NewIterator(ReadOptions());
57635787
delete it;
57645788
});

db/db_test2.cc

+13-1
Original file line numberDiff line numberDiff line change
@@ -1822,14 +1822,26 @@ TEST_F(DBTest2, ReadAmpBitmapLiveInCacheAfterDBClose) {
18221822
{
18231823
const int kIdBufLen = 100;
18241824
char id_buf[kIdBufLen];
1825+
#ifndef OS_WIN
1826+
// You can't open a directory on windows using random access file
18251827
std::unique_ptr<RandomAccessFile> file;
1826-
env_->NewRandomAccessFile(dbname_, &file, EnvOptions());
1828+
ASSERT_OK(env_->NewRandomAccessFile(dbname_, &file, EnvOptions()));
18271829
if (file->GetUniqueId(id_buf, kIdBufLen) == 0) {
18281830
// fs holding db directory doesn't support getting a unique file id,
18291831
// this means that running this test will fail because lru_cache will load
18301832
// the blocks again regardless of them being already in the cache
18311833
return;
18321834
}
1835+
#else
1836+
std::unique_ptr<Directory> dir;
1837+
ASSERT_OK(env_->NewDirectory(dbname_, &dir));
1838+
if (dir->GetUniqueId(id_buf, kIdBufLen) == 0) {
1839+
// fs holding db directory doesn't support getting a unique file id,
1840+
// this means that running this test will fail because lru_cache will load
1841+
// the blocks again regardless of them being already in the cache
1842+
return;
1843+
}
1844+
#endif
18331845
}
18341846
uint32_t bytes_per_bit[2] = {1, 16};
18351847
for (size_t k = 0; k < 2; k++) {

env/env_test.cc

+31
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,37 @@ TEST_F(EnvPosixTest, RunImmediately) {
135135
}
136136
}
137137

138+
#ifdef OS_WIN
139+
TEST_F(EnvPosixTest, AreFilesSame) {
140+
{
141+
bool tmp;
142+
if (env_->AreFilesSame("", "", &tmp).IsNotSupported()) {
143+
fprintf(stderr,
144+
"skipping EnvBasicTestWithParam.AreFilesSame due to "
145+
"unsupported Env::AreFilesSame\n");
146+
return;
147+
}
148+
}
149+
150+
const EnvOptions soptions;
151+
auto* env = Env::Default();
152+
std::string same_file_name = test::TmpDir(env) + "/same_file";
153+
std::string same_file_link_name = same_file_name + "_link";
154+
155+
std::unique_ptr<WritableFile> same_file;
156+
ASSERT_OK(env->NewWritableFile(same_file_name,
157+
&same_file, soptions));
158+
same_file->Append("random_data");
159+
ASSERT_OK(same_file->Flush());
160+
same_file.reset();
161+
162+
ASSERT_OK(env->LinkFile(same_file_name, same_file_link_name));
163+
bool result = false;
164+
ASSERT_OK(env->AreFilesSame(same_file_name, same_file_link_name, &result));
165+
ASSERT_TRUE(result);
166+
}
167+
#endif
168+
138169
TEST_P(EnvPosixTestWithParam, UnSchedule) {
139170
std::atomic<bool> called(false);
140171
env_->SetBackgroundThreads(1, Env::LOW);

include/rocksdb/env.h

+4
Original file line numberDiff line numberDiff line change
@@ -802,6 +802,10 @@ class Directory {
802802
virtual ~Directory() {}
803803
// Fsync directory. Can be called concurrently from multiple threads.
804804
virtual Status Fsync() = 0;
805+
806+
virtual size_t GetUniqueId(char* id, size_t max_size) const {
807+
return 0;
808+
}
805809
};
806810

807811
enum InfoLogLevel : unsigned char {

0 commit comments

Comments
 (0)