Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows cumulative patch #3552

Closed
wants to merge 15 commits into from

Conversation

yuslepukhin
Copy link
Contributor

@yuslepukhin yuslepukhin commented Mar 2, 2018

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

@facebook-github-bot
Copy link
Contributor

@yuslepukhin has updated the pull request. View: changes

@facebook-github-bot
Copy link
Contributor

@yuslepukhin has updated the pull request. View: changes

@facebook-github-bot
Copy link
Contributor

@yuslepukhin has updated the pull request. View: changes

@facebook-github-bot
Copy link
Contributor

@yuslepukhin has updated the pull request.

@yuslepukhin
Copy link
Contributor Author

@yiwu-arbug @siying @maysamyabandeh @anand1976 Pls, take a look. This is fairly straight forward.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@siying
Copy link
Contributor

siying commented Mar 5, 2018

Looks good to me. I'm going to merge it after tests pass.

void OnStallConditionsChanged(const WriteStallInfo& info) override {
MutexLock l(&mutex_);
condition_ = info.condition.cur;
if (expected_set_ &&
condition_ == expected_) {
cond_.Signal();
Copy link
Contributor

Choose a reason for hiding this comment

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

@yuslepukhin ASAN complains this:

Failed to find test summary. This test binary may have crashed mid-run. Full output follows:
Note: Google Test filter = DBTest.SoftLimit
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DBTest
[ RUN      ] DBTest.SoftLimit
internal_repo_rocksdb/repo/db/db_test.cc:5391:9: runtime error: load of value 190, which is not a valid value for type 'bool'
    #0 0x681976 in rocksdb::WriteStallListener::OnStallConditionsChanged(rocksdb::WriteStallInfo const&) internal_repo_rocksdb/repo/db/db_test.cc:5391
    #1 0x7f7cf9d91494 in rocksdb::SuperVersionContext::Clean() internal_repo_rocksdb/repo/db/job_context.h:63
    #2 0x7f7cf9d826e2 in rocksdb::JobContext::Clean() internal_repo_rocksdb/repo/db/job_context.h:162
    #3 0x7f7cf9f33d83 in rocksdb::DBImpl::BackgroundCallFlush() internal_repo_rocksdb/repo/db/db_impl_compaction_flush.cc:1404
    #4 0x7f7cf9f30fe5 in rocksdb::DBImpl::BGWorkFlush(void*) internal_repo_rocksdb/repo/db/db_impl_compaction_flush.cc:1250
    #5 0x7f7cfa6af0f1 in rocksdb::ThreadPoolImpl::Schedule(void (*)(void*), void*, void*, void (*)(void*))::$_1::operator()() const internal_repo_rocksdb/repo/util/threadpool_imp.cc:441
    #6 0x7f7cfa6aeefc in std::_Function_handler<void (), rocksdb::ThreadPoolImpl::Schedule(void (*)(void*), void*, void*, void (*)(void*))::$_1>::_M_invoke(std::_Any_data const&) third-party-buck/gcc-5-glibc-2.23/build/libgcc/include/c++/trunk/functional:1871
    #7 0x7f7cfa18325e in std::function<void ()>::operator()() const third-party-buck/gcc-5-glibc-2.23/build/libgcc/include/c++/trunk/functional:2267
    #8 0x7f7cfa6ab557 in rocksdb::ThreadPoolImpl::Impl::BGThread(unsigned long) internal_repo_rocksdb/repo/util/threadpool_imp.cc:240
    #9 0x7f7cfa6ab839 in rocksdb::ThreadPoolImpl::Impl::BGThreadWrapper(void*) internal_repo_rocksdb/repo/util/threadpool_imp.cc:278
    #10 0x7f7cfa6b560e in void* std::_Bind_simple<void* (*(rocksdb::BGThreadMetadata*))(void*)>::_M_invoke<0ul>(std::_Index_tuple<0ul>) third-party-buck/gcc-5-glibc-2.23/build/libgcc/include/c++/trunk/functional:1530
    #11 0x7f7cfa6b5574 in std::_Bind_simple<void* (*(rocksdb::BGThreadMetadata*))(void*)>::operator()() third-party-buck/gcc-5-glibc-2.23/build/libgcc/include/c++/trunk/functional:1520
    #12 0x7f7cfa6b52a8 in std::thread::_Impl<std::_Bind_simple<void* (*(rocksdb::BGThreadMetadata*))(void*)> >::_M_run() third-party-buck/gcc-5-glibc-2.23/build/libgcc/include/c++/trunk/thread:115
    #13 0x7f7cf6e56170 in execute_native_thread_routine /home/engshare/third-party2/libgcc/5.x/src/gcc-5/x86_64-facebook-linux/libstdc++-v3/src/c++11/../../../.././libstdc++-v3/src/c++11/thread.cc:84
    #14 0x7f7cf4cab7a8 in start_thread /home/engshare/third-party2/glibc/2.23/src/glibc-2.23/nptl/pthread_create.c:333
    #15 0x7f7cf44d7a7c in __clone /home/engshare/third-party2/glibc/2.23/src/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

I assume it's because expected_set_ has never been initialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siying My bad.

@facebook-github-bot
Copy link
Contributor

@yuslepukhin has updated the pull request.

@facebook-github-bot
Copy link
Contributor

@yuslepukhin has updated the pull request.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@yuslepukhin
Copy link
Contributor Author

@siying Travis failed due to time limit although there are some more badly written tests that now fail after rebase. This check in also ensures that AppVeyour fails when test fails.

@yuslepukhin
Copy link
Contributor Author

@siying Found one more issue

@facebook-github-bot
Copy link
Contributor

@yuslepukhin has updated the pull request. View: changes, changes since last import

@yuslepukhin
Copy link
Contributor Author

@siying Would it be possible to make the build fail on std::thread reference instead of port::Thread?

@siying
Copy link
Contributor

siying commented Mar 6, 2018

@yuslepukhin sure. I don't know how to do it though.
btw, our internal CI system is crowded so build for this PR didn't start yet. I'll check tomorrow morning and merge if it passes.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@yuslepukhin yuslepukhin deleted the windows_cumulative branch August 7, 2018 22:06
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.

None yet

3 participants