-
Notifications
You must be signed in to change notification settings - Fork 165
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
Fix mac build #41
Fix mac build #41
Conversation
JiayuZzz
commented
Jul 10, 2019
•
edited by yiwu-arbug
Loading
edited by yiwu-arbug
- There is no libatomic in MacOS clang(and maybe some other environment), so check if libatomic existed before link.
- Added MacOS build to travis. For TSAN build , a unused-command-line-argument is defined in RocksDB cmake file, witch will fail compilation on MacOS, skip it.
src/blob_file_iterator.cc
Outdated
@@ -170,8 +170,10 @@ void BlobFileMergeIterator::Next() { | |||
assert(current_ != nullptr); | |||
current_->Next(); | |||
if (current_->status().ok() && current_->Valid()) min_heap_.push(current_); | |||
current_ = min_heap_.top(); | |||
min_heap_.pop(); | |||
if (!min_heap_.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a unit test for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow. how this didn't cause data corruption? looks scary. @Connor1996
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yiwu-arbug The behavior of top()
when queue is empty is undefined. In our environment, the return value may be nullptr
, so everything works fine. And in macOS, it doesn't so fail.
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 84.12% 84.14% +0.01%
==========================================
Files 43 43
Lines 2577 2579 +2
==========================================
+ Hits 2168 2170 +2
Misses 409 409 |
.gitignore
Outdated
@@ -8,3 +8,4 @@ titandb_stress | |||
build/ | |||
.idea/ | |||
.vscode/ | |||
cmake-build-debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yiwu-arbug It's CLion generated folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a slash at the end?
CMakeLists.txt
Outdated
@@ -79,8 +83,11 @@ if (WITH_TITAN_TOOLS) | |||
else() | |||
# Hack: On Travis (with Ubuntu xenial or before), libgflags-dev package doesn't come with | |||
# gflags-config.cmake, so find_package will fail. Hard-code gflag path for now. | |||
set(gflags_INCLUDE_DIR "/usr/include/gflags") | |||
list(APPEND TOOLS_LIBS "/usr/lib/x86_64-linux-gnu/libgflags.a") | |||
if(APPLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (NOT APPLE)
CMakeLists.txt
Outdated
@@ -38,7 +38,11 @@ endif() | |||
if (WITH_TITAN_TESTS OR WITH_TITAN_TOOLS) | |||
add_subdirectory(${ROCKSDB_DIR} rocksdb EXCLUDE_FROM_ALL) | |||
# -latomic is needed when building titandb_stress using clang. | |||
link_libraries(atomic) | |||
# check if libatomic exist before link | |||
find_library(ATOMIC_EXIST NAMES atomic libatomic.so.1 atomic.so.1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/ATOMIC_EXIST/HAS_ATOMIC/
.travis.yml
Outdated
|
||
install: | ||
# osx dependencies | ||
- if [ "${TRAVIS_OS_NAME}" == osx ]; then | ||
brew install gflags zstd lz4 snappy xz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indent
- compiler: clang | ||
# For osx, there is a unused-command-line-argument error in RocksDB cmake file with TSAN | ||
exclude: | ||
- os: osx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not running any sanitizer build for Mac build then.
src/blob_file_iterator.cc
Outdated
@@ -96,7 +96,7 @@ void BlobFileIterator::GetBlobRecord() { | |||
|
|||
Slice record_slice; | |||
auto record_size = decoder_.GetRecordSize(); | |||
buffer_.reserve(record_size); | |||
buffer_.resize(record_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch.
src/blob_file_iterator_test.cc
Outdated
} | ||
ASSERT_EQ(i, kMaxKeyNum); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the test fail before your fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems it can pass even without the fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wujy-cs said the test will fail ASAN on Mac though.
src/blob_file_iterator.cc
Outdated
if (!min_heap_.empty()) { | ||
current_ = min_heap_.top(); | ||
min_heap_.pop(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else { current_ = nullptr; }
src/blob_file_iterator.cc
Outdated
@@ -170,8 +170,10 @@ void BlobFileMergeIterator::Next() { | |||
assert(current_ != nullptr); | |||
current_->Next(); | |||
if (current_->status().ok() && current_->Valid()) min_heap_.push(current_); | |||
current_ = min_heap_.top(); | |||
min_heap_.pop(); | |||
if (!min_heap_.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow. how this didn't cause data corruption? looks scary. @Connor1996
src/blob_file_iterator.cc
Outdated
@@ -170,8 +170,10 @@ void BlobFileMergeIterator::Next() { | |||
assert(current_ != nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert(Valid());
Please extract blob_file_iterator fix to a single PR |
src/blob_file_iterator_test.cc
Outdated
@@ -209,13 +209,17 @@ TEST_F(BlobFileIteratorTest, MergeIterator) { | |||
BlobFileMergeIterator iter(std::move(iters)); | |||
|
|||
iter.SeekToFirst(); | |||
for (int i = 1; i < kMaxKeyNum; i++, iter.Next()) { | |||
int i = 1; | |||
while (iter.Valid()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while (iter.Valid()) { | |
for (;iter.Valid(); i++, iter.Next()) { |
seems more clean
LGTM. Please address @Connor1996's comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM