You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by ad...@apache.org on 2018/05/18 02:52:28 UTC
[1/3] kudu git commit: [consensus] KUDU-2443 fix replica replacement
of RF=1
Repository: kudu
Updated Branches:
refs/heads/master cfff4bb18 -> 185781f6c
[consensus] KUDU-2443 fix replica replacement of RF=1
Fixed bug in the consensus::ShouldEvictReplica() function for the case
when the leader replica of a tablet with replication factor of 1
is marked with the REPLACE attribute and there is an extra voter
replica in the tablet's Raft config.
Prior to this fix, the master would evict the extra voter from the
configuration and then it would add a new non-voter because of the
consensus::ShouldAddReplica() method's behavior. After the newly
added non-voter replica catches up and becomes a voter, that would
happen again and again, until the REPLACE attribute is removed.
This changelist also includes regression tests to cover the
corresponding functionality. Also, additional test scenarios added
to extend the coverage for the single-replica edge case.
Change-Id: I9da9fe6788f28b40f7adc53e23540bcdf103c1ea
Reviewed-on: http://gerrit.cloudera.org:8080/10438
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/cb203729
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/cb203729
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/cb203729
Branch: refs/heads/master
Commit: cb203729255dba06120d2a2f8702032a2ffd9694
Parents: cfff4bb
Author: Alexey Serbin <as...@cloudera.com>
Authored: Wed May 16 17:35:31 2018 -0700
Committer: Alexey Serbin <as...@cloudera.com>
Committed: Fri May 18 02:41:30 2018 +0000
----------------------------------------------------------------------
src/kudu/consensus/quorum_util-test.cc | 83 ++++++++++++++++++++++++++++-
src/kudu/consensus/quorum_util.cc | 4 ++
2 files changed, 86 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/cb203729/src/kudu/consensus/quorum_util-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util-test.cc b/src/kudu/consensus/quorum_util-test.cc
index f98e819..46776e0 100644
--- a/src/kudu/consensus/quorum_util-test.cc
+++ b/src/kudu/consensus/quorum_util-test.cc
@@ -15,7 +15,10 @@
// specific language governing permissions and limitations
// under the License.
+#include "kudu/consensus/quorum_util.h"
+
#include <memory>
+#include <ostream>
#include <string>
#include <utility>
#include <vector>
@@ -25,7 +28,6 @@
#include "kudu/common/common.pb.h"
#include "kudu/consensus/metadata.pb.h"
-#include "kudu/consensus/quorum_util.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/util/status.h"
#include "kudu/util/test_macros.h"
@@ -73,6 +75,21 @@ static void SetOverallHealth(HealthReportPB* health_report,
}
}
+std::ostream& operator<<(std::ostream& os, MajorityHealthPolicy policy) {
+ switch (policy) {
+ case MajorityHealthPolicy::HONOR:
+ os << "MajorityHealthPolicy::HONOR";
+ break;
+ case MajorityHealthPolicy::IGNORE:
+ os << "MajorityHealthPolicy::IGNORE";
+ break;
+ default:
+ os << policy << ": unsupported health policy";
+ break;
+ }
+ return os;
+}
+
// Add a consensus peer into the specified configuration.
static void AddPeer(RaftConfigPB* config,
const string& uuid,
@@ -1057,6 +1074,62 @@ TEST_P(QuorumUtilHealthPolicyParamTest, ReplaceAttributeBasic) {
{
RaftConfigPB config;
AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ EXPECT_TRUE(ShouldAddReplica(config, 1, policy));
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ }
+ {
+ // Regression test scenario for KUDU-2443.
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", V, '+');
+ EXPECT_FALSE(ShouldAddReplica(config, 1, policy));
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ string to_evict;
+ ASSERT_TRUE(ShouldEvictReplica(config, "B", 1, policy, &to_evict));
+ EXPECT_EQ("A", to_evict);
+ }
+ {
+ for (auto health_status : { '+', '-', '?', 'x' }) {
+ SCOPED_TRACE(Substitute("health status '$0'", health_status));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", N, health_status);
+ EXPECT_TRUE(ShouldAddReplica(config, 1, policy));
+ if (health_status == '+' || health_status == '?') {
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ } else {
+ string to_evict;
+ ASSERT_TRUE(ShouldEvictReplica(config, "A", 1, policy, &to_evict));
+ EXPECT_EQ("B", to_evict);
+ }
+ }
+ }
+ // If a non-voter replica with PROMOTE=true is already in the Raft config,
+ // no need to add an additional one if the health status of the non-voter
+ // replica is HEALTHY or UNKNOWN.
+ {
+ for (auto health_status : { '+', '-', '?', 'x' }) {
+ SCOPED_TRACE(Substitute("health status '$0'", health_status));
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
+ AddPeer(&config, "B", N, health_status, {{"PROMOTE", true}});
+ if (health_status == '+' || health_status == '?') {
+ EXPECT_FALSE(ShouldAddReplica(config, 1, policy));
+ } else {
+ EXPECT_TRUE(ShouldAddReplica(config, 1, policy));
+ }
+ if (health_status == '+' || health_status == '?') {
+ EXPECT_FALSE(ShouldEvictReplica(config, "A", 1, policy));
+ } else {
+ string to_evict;
+ ASSERT_TRUE(ShouldEvictReplica(config, "A", 1, policy, &to_evict));
+ EXPECT_EQ("B", to_evict);
+ }
+ }
+ }
+ {
+ RaftConfigPB config;
+ AddPeer(&config, "A", V, '+', {{"REPLACE", true}});
AddPeer(&config, "B", V, '+');
AddPeer(&config, "C", V, '+');
EXPECT_FALSE(ShouldEvictReplica(config, "A", 3, policy));
@@ -1070,6 +1143,13 @@ TEST_P(QuorumUtilHealthPolicyParamTest, ReplaceAttributeBasic) {
AddPeer(&config, "D", V, '+');
EXPECT_FALSE(ShouldEvictReplica(config, "A", 3, policy));
EXPECT_FALSE(ShouldAddReplica(config, 3, policy));
+
+ for (const auto& leader_replica : { "B", "C", "D" }) {
+ string to_evict;
+ SCOPED_TRACE(Substitute("leader $0", leader_replica));
+ ASSERT_TRUE(ShouldEvictReplica(config, leader_replica, 3, policy, &to_evict));
+ EXPECT_EQ("A", to_evict);
+ }
}
for (auto health_status : { '-', '?', 'x' }) {
RaftConfigPB config;
@@ -1093,6 +1173,7 @@ TEST_P(QuorumUtilHealthPolicyParamTest, ReplaceAttributeBasic) {
AddPeer(&config, "C", V, '+');
AddPeer(&config, "D", V, '+');
AddPeer(&config, "E", V, '+');
+ // There should be no attempt to evict the leader.
string to_evict;
ASSERT_TRUE(ShouldEvictReplica(config, "A", 3, policy, &to_evict));
EXPECT_NE("A", to_evict);
http://git-wip-us.apache.org/repos/asf/kudu/blob/cb203729/src/kudu/consensus/quorum_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/consensus/quorum_util.cc b/src/kudu/consensus/quorum_util.cc
index 1c0fc71..88a231c 100644
--- a/src/kudu/consensus/quorum_util.cc
+++ b/src/kudu/consensus/quorum_util.cc
@@ -732,7 +732,11 @@ bool ShouldEvictReplica(const RaftConfigPB& config,
// Working with the replicas marked with the 'replace' attribute:
// the case when too many replicas are marked with the 'replace' attribute
// while all required replicas are healthy.
+ //
+ // In the special case when the leader replica is the only one marked with the
+ // 'replace' attribute, the leader replica cannot be evicted.
need_to_evict_voter |= (num_voters_healthy >= replication_factor) &&
+ !(num_voters_with_replace == 1 && leader_with_replace) &&
((num_voters_with_replace > replication_factor) ||
(num_voters_with_replace >= replication_factor && num_voters_viable > 0));
[3/3] kudu git commit: build: only call find_package on gperftools if
we're going to use it
Posted by ad...@apache.org.
build: only call find_package on gperftools if we're going to use it
Since commit e0a743d24, TSAN builds emit the following non-fatal warning
when cmake is run:
-- Could NOT find GOOGLE_PERFTOOLS (missing: TCMALLOC_SHARED_LIB TCMALLOC_STATIC_LIB PROFILER_SHARED_LIB PROFILER_STATIC_LIB GOOGLE_PERFTOOLS_INCLUDE_DIR)
Moving the find_package() call into the gperftools condition means we won't
try to look for it at all when we're not going to use it.
Change-Id: I76b6c1244cdd5b901b6c23cb56a5613439113773
Reviewed-on: http://gerrit.cloudera.org:8080/10429
Tested-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/185781f6
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/185781f6
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/185781f6
Branch: refs/heads/master
Commit: 185781f6ced48f1b4a7216034285f772a1efb30b
Parents: 1a7f8c7
Author: Adar Dembo <ad...@cloudera.com>
Authored: Sat May 12 14:09:42 2018 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Fri May 18 02:51:56 2018 +0000
----------------------------------------------------------------------
CMakeLists.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/185781f6/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/CMakeLists.txt b/CMakeLists.txt
index fe6752e..a180a72 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1122,10 +1122,10 @@ ADD_THIRDPARTY_LIB(krb5
##
## Disabled with TSAN/ASAN as well as with gold+dynamic linking (see
## earlier comment).
-find_package(GPerf REQUIRED)
if (NOT "${KUDU_USE_ASAN}" AND
NOT "${KUDU_USE_TSAN}" AND
NOT ("${KUDU_BUGGY_GOLD}" AND "${KUDU_LINK}" STREQUAL "d"))
+ find_package(GPerf REQUIRED)
ADD_THIRDPARTY_LIB(tcmalloc
STATIC_LIB "${TCMALLOC_STATIC_LIB}"
SHARED_LIB "${TCMALLOC_SHARED_LIB}")
[2/3] kudu git commit: KUDU-2427: adjust gold linker detection
Posted by ad...@apache.org.
KUDU-2427: adjust gold linker detection
This patch makes two adjustments to the existing gold linker detection:
1. Ubuntu 18.04's gcc 7 no longer respects a /usr/bin/ld symlink; it is
hard-wired to use ld.bfd. Instead, we need to pass -fuse-ld=gold to
choose the gold linker.
2. The old bug that led to tcmalloc being dropped from Kudu's dependency
list has been fixed, so let's condition the workaround on the appropriate
version of the gold linker.
Change-Id: Ib1fae9893aaaf4916205d4c5ae6bb5c93e0505d0
Reviewed-on: http://gerrit.cloudera.org:8080/10428
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/1a7f8c78
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/1a7f8c78
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/1a7f8c78
Branch: refs/heads/master
Commit: 1a7f8c78e7c632f810c2395db4922b6a55a9b96e
Parents: cb20372
Author: Adar Dembo <ad...@cloudera.com>
Authored: Thu May 10 16:06:27 2018 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Fri May 18 02:51:24 2018 +0000
----------------------------------------------------------------------
CMakeLists.txt | 102 ++++++++++++++++++++++++++++-------
src/kudu/codegen/CMakeLists.txt | 4 ++
2 files changed, 86 insertions(+), 20 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/1a7f8c78/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 782ecbc..fe6752e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -438,27 +438,89 @@ if ("${KUDU_LINK}" STREQUAL "a")
endif()
endif()
-# Are we using the gold linker? It doesn't work with dynamic linking as
-# weak symbols aren't properly overridden, causing tcmalloc to be omitted.
-# Let's flag this as an error in RELEASE builds (we shouldn't release a
-# product like this).
+# Interrogates the linker version via the C++ compiler to determine whether
+# we're using the gold linker, and if so, extracts its version.
#
-# See https://sourceware.org/bugzilla/show_bug.cgi?id=16979 for details.
+# If the gold linker is being used, sets GOLD_VERSION in the parent scope with
+# the extracted version.
#
-# The gold linker is only for ELF binaries, which OSX doesn't use. We can
-# just skip.
-if (NOT APPLE)
- execute_process(COMMAND ${CMAKE_CXX_COMPILER} -Wl,--version OUTPUT_VARIABLE LINKER_OUTPUT)
-endif ()
-if (LINKER_OUTPUT MATCHES "gold")
- if ("${KUDU_LINK}" STREQUAL "d" AND
- "${CMAKE_BUILD_TYPE}" STREQUAL "RELEASE")
- message(SEND_ERROR "Cannot use gold with dynamic linking in a RELEASE build "
- "as it would cause tcmalloc symbols to get dropped")
+# Any additional arguments are passed verbatim into the C++ compiler invocation.
+function(GET_GOLD_VERSION)
+ # The gold linker is only for ELF binaries, which macOS doesn't use.
+ if (APPLE)
+ return()
+ endif()
+
+ execute_process(COMMAND ${CMAKE_CXX_COMPILER} "-Wl,--version" ${ARGN}
+ ERROR_QUIET
+ OUTPUT_VARIABLE LINKER_OUTPUT)
+ # We're expecting LINKER_OUTPUT to look like one of these:
+ # GNU gold (version 2.24) 1.11
+ # GNU gold (GNU Binutils for Ubuntu 2.30) 1.15
+ if (LINKER_OUTPUT MATCHES "GNU gold")
+ string(REGEX MATCH "GNU gold \\([^\\)]*\\) (([0-9]+\\.?)+)" _ "${LINKER_OUTPUT}")
+ if (NOT CMAKE_MATCH_1)
+ message(SEND_ERROR "Could not extract GNU gold version. "
+ "Linker version output: ${LINKER_OUTPUT}")
+ endif()
+ set(GOLD_VERSION "${CMAKE_MATCH_1}" PARENT_SCOPE)
+ endif()
+endfunction()
+
+# Is the compiler hard-wired to use the gold linker?
+GET_GOLD_VERSION()
+if (GOLD_VERSION)
+ set(MUST_USE_GOLD 1)
+else()
+ # Can the compiler optionally enable the gold linker?
+ GET_GOLD_VERSION("-fuse-ld=gold")
+
+ # We can't use the gold linker if it's inside devtoolset because the compiler
+ # won't find it when invoked directly from make/ninja (which is typically
+ # done outside devtoolset).
+ execute_process(COMMAND which ld.gold
+ OUTPUT_VARIABLE GOLD_LOCATION
+ OUTPUT_STRIP_TRAILING_WHITESPACE
+ ERROR_QUIET)
+ if ("${GOLD_LOCATION}" MATCHES "^/opt/rh/devtoolset")
+ message("Skipping optional gold linker (version ${GOLD_VERSION}) because "
+ "it's in devtoolset")
+ set(GOLD_VERSION)
+ endif()
+endif()
+if (GOLD_VERSION)
+ # Older versions of the gold linker are vulnerable to a bug [1] which
+ # prevents weak symbols from being overridden properly. This leads to
+ # omitting of Kudu's tcmalloc dependency.
+ #
+ # How we handle this situation depends on other factors:
+ # - If gold is optional, we won't use it.
+ # - If gold is required, we'll either:
+ # - Raise an error in RELEASE builds (we shouldn't release such a product), or
+ # - Drop tcmalloc in all other builds.
+ #
+ # 1. https://sourceware.org/bugzilla/show_bug.cgi?id=16979.
+ if ("${GOLD_VERSION}" VERSION_LESS "1.12")
+ set(KUDU_BUGGY_GOLD 1)
+ endif()
+ if (MUST_USE_GOLD)
+ message("Using hard-wired gold linker (version ${GOLD_VERSION})")
+ if (KUDU_BUGGY_GOLD)
+ if ("${KUDU_LINK}" STREQUAL "d" AND
+ "${CMAKE_BUILD_TYPE}" STREQUAL "RELEASE")
+ message(SEND_ERROR "Configured to use buggy gold with dynamic linking "
+ "in a RELEASE build; this would cause tcmalloc symbols to get dropped")
+ endif()
+ message("Hard-wired gold linker is buggy, dropping tcmalloc dependency")
+ endif()
+ elseif (NOT KUDU_BUGGY_GOLD)
+ # The Gold linker must be manually enabled.
+ set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fuse-ld=gold")
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fuse-ld=gold")
+ message("Using optional gold linker (version ${GOLD_VERSION})")
else()
- message("Using gold linker")
+ message("Optional gold linker is buggy, using ld linker instead")
endif()
- set(KUDU_USING_GOLD 1)
else()
message("Using ld linker")
endif()
@@ -1058,12 +1120,12 @@ ADD_THIRDPARTY_LIB(krb5
## Google PerfTools
##
-## Disabled with TSAN/ASAN as well as with gold+dynamic linking (see comment
-## near definition of KUDU_USING_GOLD).
+## Disabled with TSAN/ASAN as well as with gold+dynamic linking (see
+## earlier comment).
find_package(GPerf REQUIRED)
if (NOT "${KUDU_USE_ASAN}" AND
NOT "${KUDU_USE_TSAN}" AND
- NOT ("${KUDU_USING_GOLD}" AND "${KUDU_LINK}" STREQUAL "d"))
+ NOT ("${KUDU_BUGGY_GOLD}" AND "${KUDU_LINK}" STREQUAL "d"))
ADD_THIRDPARTY_LIB(tcmalloc
STATIC_LIB "${TCMALLOC_STATIC_LIB}"
SHARED_LIB "${TCMALLOC_SHARED_LIB}")
http://git-wip-us.apache.org/repos/asf/kudu/blob/1a7f8c78/src/kudu/codegen/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/kudu/codegen/CMakeLists.txt b/src/kudu/codegen/CMakeLists.txt
index 3cd3300..c8bbbb0 100644
--- a/src/kudu/codegen/CMakeLists.txt
+++ b/src/kudu/codegen/CMakeLists.txt
@@ -125,6 +125,10 @@ list(REMOVE_ITEM IR_FLAGS "-fsanitize=thread" "-DTHREAD_SANITIZER")
# again at runtime.
list(REMOVE_ITEM IR_FLAGS "-O3" "-O2" "-O1" "-Os")
+# If present (see the top-level CMakeLists.txt), this has no effect and just generates
+# an unused argument warning.
+list(REMOVE_ITEM IR_FLAGS "-fuse-ld=gold")
+
# Disable built-in LLVM passes which would add 'noinline' attributes to all
# standalone functions.
#