Fix mac build #41

JiayuZzz · 2019-07-10T16:32:49Z

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.

CLAassistant · 2019-07-10T16:32:52Z

All committers have signed the CLA.

Connor1996 · 2019-07-11T00:43:35Z

src/blob_file_iterator.cc

@@ -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()) {


Please add a unit test for this

Ok, it's done.

oh wow. how this didn't cause data corruption? looks scary. @Connor1996

@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-io · 2019-07-11T01:10:51Z

Codecov Report

Merging #41 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            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

yiwu-arbug · 2019-07-11T04:35:42Z

.gitignore

@@ -8,3 +8,4 @@ titandb_stress
 build/
 .idea/
 .vscode/
+cmake-build-debug


what's this?

@yiwu-arbug It's CLion generated folder

add a slash at the end?

yiwu-arbug · 2019-07-11T04:37:21Z

CMakeLists.txt

@@ -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)


if (NOT APPLE)

yiwu-arbug · 2019-07-11T04:38:09Z

CMakeLists.txt

@@ -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)


s/ATOMIC_EXIST/HAS_ATOMIC/

yiwu-arbug · 2019-07-11T04:41:01Z

.travis.yml


 install:
+  # osx dependencies
+  - if [ "${TRAVIS_OS_NAME}" == osx ]; then
+    brew install gflags zstd lz4 snappy xz;


nit: indent

yiwu-arbug · 2019-07-11T04:46:49Z

.travis.yml

-    - compiler: clang
+  # For osx, there is a unused-command-line-argument error in RocksDB cmake file with TSAN
+  exclude:
+    - os: osx


Let's not running any sanitizer build for Mac build then.

yiwu-arbug · 2019-07-11T04:49:12Z

src/blob_file_iterator.cc

@@ -96,7 +96,7 @@ void BlobFileIterator::GetBlobRecord() {

  Slice record_slice;
  auto record_size = decoder_.GetRecordSize();
-  buffer_.reserve(record_size);
+  buffer_.resize(record_size);


good catch.

yiwu-arbug · 2019-07-11T04:59:29Z

src/blob_file_iterator_test.cc

  }
+  ASSERT_EQ(i, kMaxKeyNum);


Does the test fail before your fix?

seems it can pass even without the fix

@wujy-cs said the test will fail ASAN on Mac though.

yiwu-arbug · 2019-07-11T05:06:09Z

src/blob_file_iterator.cc

+  if (!min_heap_.empty()) {
+    current_ = min_heap_.top();
+    min_heap_.pop();
+  }


else { current_ = nullptr; }

yiwu-arbug · 2019-07-11T05:08:44Z

src/blob_file_iterator.cc

@@ -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()) {


oh wow. how this didn't cause data corruption? looks scary. @Connor1996

yiwu-arbug · 2019-07-11T05:33:47Z

src/blob_file_iterator.cc

@@ -170,8 +170,10 @@ void BlobFileMergeIterator::Next() {
  assert(current_ != nullptr);


assert(Valid());

Connor1996 · 2019-07-16T03:52:25Z

Please extract blob_file_iterator fix to a single PR

Connor1996 · 2019-07-16T04:07:03Z

src/blob_file_iterator_test.cc

@@ -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()) {


Suggested change

while (iter.Valid()) {

for (;iter.Valid(); i++, iter.Next()) {

seems more clean

yiwu-arbug · 2019-07-16T04:48:04Z

LGTM. Please address @Connor1996's comments.

yiwu-arbug

LGTM

JiayuZzz force-pushed the master branch from 0560f4d to df4af44 Compare July 10, 2019 17:10

Connor1996 reviewed Jul 11, 2019

View reviewed changes

JiayuZzz mentioned this pull request Jul 11, 2019

Fix Mac build #34

Closed

yiwu-arbug reviewed Jul 11, 2019

View reviewed changes

disksing added the type/enhancement Type: Issue - Enhancement label Jul 11, 2019

Connor1996 reviewed Jul 16, 2019

View reviewed changes

JiayuZzz added 5 commits July 16, 2019 14:01

Fix mac build and blob_file_iterator

c4daef3

format code

838ad29

unit test

55d20da

modified

42b03b1

separate iterator fix to another pr and merge upstream

fd3b408

JiayuZzz force-pushed the master branch from 81cfa76 to fd3b408 Compare July 16, 2019 06:07

JiayuZzz changed the title ~~Fix mac build and blob_file_iterator~~ Fix mac build Jul 16, 2019

fmt

07990db

yiwu-arbug approved these changes Jul 16, 2019

View reviewed changes

yiwu-arbug merged commit 53c1096 into tikv:master Jul 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mac build #41

Fix mac build #41

JiayuZzz commented Jul 10, 2019 •

edited by yiwu-arbug

CLAassistant commented Jul 10, 2019 •

edited

Connor1996 Jul 11, 2019

JiayuZzz Jul 11, 2019

yiwu-arbug Jul 11, 2019

Connor1996 Jul 16, 2019 •

edited

codecov-io commented Jul 11, 2019 •

edited

yiwu-arbug Jul 11, 2019

JiayuZzz Jul 11, 2019

yiwu-arbug Jul 16, 2019

yiwu-arbug Jul 11, 2019

yiwu-arbug Jul 11, 2019

yiwu-arbug Jul 11, 2019

yiwu-arbug Jul 11, 2019

yiwu-arbug Jul 11, 2019

yiwu-arbug Jul 11, 2019

Connor1996 Jul 16, 2019

yiwu-arbug Jul 16, 2019

yiwu-arbug Jul 11, 2019

yiwu-arbug Jul 11, 2019

yiwu-arbug Jul 11, 2019

Connor1996 commented Jul 16, 2019

Connor1996 Jul 16, 2019

yiwu-arbug commented Jul 16, 2019

yiwu-arbug left a comment

		@@ -170,8 +170,10 @@ void BlobFileMergeIterator::Next() {
		assert(current_ != nullptr);

	while (iter.Valid()) {
	for (;iter.Valid(); i++, iter.Next()) {

Fix mac build #41

Fix mac build #41

Conversation

JiayuZzz commented Jul 10, 2019 • edited by yiwu-arbug

CLAassistant commented Jul 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 Jul 16, 2019 • edited

Choose a reason for hiding this comment

codecov-io commented Jul 11, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 commented Jul 16, 2019

Choose a reason for hiding this comment

yiwu-arbug commented Jul 16, 2019

yiwu-arbug left a comment

Choose a reason for hiding this comment

JiayuZzz commented Jul 10, 2019 •

edited by yiwu-arbug

CLAassistant commented Jul 10, 2019 •

edited

Connor1996 Jul 16, 2019 •

edited

codecov-io commented Jul 11, 2019 •

edited