kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From a...@apache.org
Subject [1/2] kudu git commit: Fix block_manager-test running in some builds
Date Tue, 02 Aug 2016 20:39:53 GMT
Repository: kudu
Updated Branches:
  refs/heads/master fee3f672b -> 9650ac7fa


Fix block_manager-test running in some builds

Even though the patch for KUDU-1538 passed pre-commit, it failed in the RELEASE
builds post-commit.

The issue appears to be that my trickery with weak symbols only worked in
some cases, depending on the link order, for dynamically linked builds,
since weak symbols are actually only relevant for statically linked ones.
Apparently in the precommit build, the link order was such that the gtest
detection did not work on block_manager-test, and thus things were fine.
But, in the release static build post-commit, the gtest detection worked,
and it broke the test in question.

This patch changes the gtest detection to use dlsym() instead of a weak symbol.
It appears to work more reliably in all types of builds. This also fixes the
test to properly manipulate the block ID sequence in the test that was failing.

Change-Id: I1a4025a3d859b139b74be997706bd27d0ba7b292
Reviewed-on: http://gerrit.cloudera.org:8080/3733
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <adar@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/a9265d92
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/a9265d92
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/a9265d92

Branch: refs/heads/master
Commit: a9265d92424dc305c480d6c44d006b8545bd2227
Parents: fee3f67
Author: Todd Lipcon <todd@apache.org>
Authored: Sun Jul 24 09:37:45 2016 -0700
Committer: Adar Dembo <adar@cloudera.com>
Committed: Tue Aug 2 20:33:38 2016 +0000

----------------------------------------------------------------------
 CMakeLists.txt                    |  7 +++++++
 src/kudu/fs/block_manager-test.cc |  9 +++++++--
 src/kudu/fs/log_block_manager.cc  |  5 ++---
 src/kudu/util/CMakeLists.txt      |  2 ++
 src/kudu/util/test_util.cc        |  6 +++---
 src/kudu/util/test_util_prod.cc   | 28 ++++++++++++++++++++++++++++
 src/kudu/util/test_util_prod.h    | 32 ++++++++++++++++++++++++++++++++
 7 files changed, 81 insertions(+), 8 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/a9265d92/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 055817d..01c9f3b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -875,6 +875,13 @@ if (NOT APPLE)
   endif()
   ADD_THIRDPARTY_LIB(rt
     SHARED_LIB "${RT_LIB_PATH}")
+
+  find_library(DL_LIB_PATH dl)
+  if(NOT DL_LIB_PATH)
+    message(FATAL_ERROR "Could not find libdl on the system path")
+  endif()
+  ADD_THIRDPARTY_LIB(dl
+    SHARED_LIB "${DL_LIB_PATH}")
 endif()
 
 ## Boost

http://git-wip-us.apache.org/repos/asf/kudu/blob/a9265d92/src/kudu/fs/block_manager-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/block_manager-test.cc b/src/kudu/fs/block_manager-test.cc
index 8a0385e..7bb53f0 100644
--- a/src/kudu/fs/block_manager-test.cc
+++ b/src/kudu/fs/block_manager-test.cc
@@ -720,9 +720,14 @@ class LogBlockManagerTest : public BlockManagerTest<LogBlockManager>
{
 // reused.
 TEST_F(LogBlockManagerTest, TestReuseBlockIds) {
   RETURN_NOT_LOG_BLOCK_MANAGER();
+
+  // Typically, the LBM starts with a random block ID when running as a
+  // gtest. In this test, we want to control the block IDs.
+  bm_->next_block_id_.Store(1);
+
   vector<BlockId> block_ids;
 
-  // Create 4 containers, with the first four block IDs in the random sequence.
+  // Create 4 containers, with the first four block IDs in the sequence.
   {
     ScopedWritableBlockCloser closer;
     for (int i = 0; i < 4; i++) {
@@ -748,7 +753,7 @@ TEST_F(LogBlockManagerTest, TestReuseBlockIds) {
     ASSERT_OK(bm_->DeleteBlock(b));
   }
 
-  // Reset the random seed and re-create new blocks which should reuse the same
+  // Reset the block ID sequence and re-create new blocks which should reuse the same
   // block IDs. This isn't allowed in current versions of Kudu, but older versions
   // could produce this situation, and we still need to handle it on startup.
   bm_->next_block_id_.Store(1);

http://git-wip-us.apache.org/repos/asf/kudu/blob/a9265d92/src/kudu/fs/log_block_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/fs/log_block_manager.cc b/src/kudu/fs/log_block_manager.cc
index 7890964..5895751 100644
--- a/src/kudu/fs/log_block_manager.cc
+++ b/src/kudu/fs/log_block_manager.cc
@@ -41,6 +41,7 @@
 #include "kudu/util/pb_util.h"
 #include "kudu/util/random_util.h"
 #include "kudu/util/stopwatch.h"
+#include "kudu/util/test_util_prod.h"
 #include "kudu/util/threadpool.h"
 #include "kudu/util/trace.h"
 
@@ -112,8 +113,6 @@ using strings::Substitute;
 
 namespace kudu {
 
-ATTRIBUTE_WEAK bool g_is_gtest;
-
 namespace fs {
 namespace internal {
 
@@ -1102,7 +1101,7 @@ LogBlockManager::LogBlockManager(Env* env, const BlockManagerOptions&
opts)
   // likely that the block IDs are not reused. So, instead of starting with
   // block ID 1, we'll start with a random block ID. A collision is still
   // possible, but exceedingly unlikely.
-  if (g_is_gtest) {
+  if (IsGTest()) {
     Random r(GetRandomSeed32());
     next_block_id_.Store(r.Next64());
   }

http://git-wip-us.apache.org/repos/asf/kudu/blob/a9265d92/src/kudu/util/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/util/CMakeLists.txt b/src/kudu/util/CMakeLists.txt
index 80ee2f3..361209c 100644
--- a/src/kudu/util/CMakeLists.txt
+++ b/src/kudu/util/CMakeLists.txt
@@ -167,6 +167,7 @@ set(UTIL_SRCS
   subprocess.cc
   sync_point.cc
   test_graph.cc
+  test_util_prod.cc
   thread.cc
   threadlocal.cc
   threadpool.cc
@@ -192,6 +193,7 @@ endif()
 
 set(UTIL_LIBS
   crcutil
+  dl
   gflags
   glog
   gutil

http://git-wip-us.apache.org/repos/asf/kudu/blob/a9265d92/src/kudu/util/test_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index 46f4714..58be8ff 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -45,9 +45,9 @@ static const uint64 kTestBeganAtMicros = Env::Default()->NowMicros();
 
 // Global which production code can check to see if it is running
 // in a GTest environment (assuming the test binary links in this module,
-// which is typically a good assumption). This can be imported
-// as a weak symbol such that, if it's not linked in, the reader sees
-// 'false'.
+// which is typically a good assumption).
+//
+// This can be checked using the 'IsGTest()' function from test_util_prod.cc.
 bool g_is_gtest = true;
 
 ///////////////////////////////////////////////////

http://git-wip-us.apache.org/repos/asf/kudu/blob/a9265d92/src/kudu/util/test_util_prod.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util_prod.cc b/src/kudu/util/test_util_prod.cc
new file mode 100644
index 0000000..5523bac
--- /dev/null
+++ b/src/kudu/util/test_util_prod.cc
@@ -0,0 +1,28 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "kudu/util/test_util_prod.h"
+
+#include <dlfcn.h>
+
+namespace kudu {
+
+bool IsGTest() {
+  return dlsym(RTLD_DEFAULT, "_ZN4kudu10g_is_gtestE") != nullptr;
+}
+
+} // namespace kudu

http://git-wip-us.apache.org/repos/asf/kudu/blob/a9265d92/src/kudu/util/test_util_prod.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util_prod.h b/src/kudu/util/test_util_prod.h
new file mode 100644
index 0000000..8b7ea61
--- /dev/null
+++ b/src/kudu/util/test_util_prod.h
@@ -0,0 +1,32 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Test-related utility methods that can be called from non-test
+// code. This module is part of the 'util' module and is built into
+// all binaries, not just tests, whereas 'test_util.cc' is linked
+// only into test binaries.
+
+#pragma once
+
+namespace kudu {
+
+// Return true if the current binary is a gtest. More specifically,
+// returns true if the 'test_util.cc' module has been linked in
+// (either dynamically or statically) to the running process.
+bool IsGTest();
+
+} // namespace kudu


Mime
View raw message