This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push: new 45a7857b4 [rpc] report the names of the unsupported feature flags 45a7857b4 is described below commit 45a7857b494703ef5cf9da8671c75da88d5fc16d Author: Alexey Serbin <ale...@apache.org> AuthorDate: Thu Jun 16 19:16:34 2022 -0700 [rpc] report the names of the unsupported feature flags While introducing feature flags in the scope of KUDU-2671, I noticed that missing feature flags are not reported in the error messages (at least, I was looking at master_proxy_rpc.cc). This patch addresses that, making corresponding error messages more actionable. As for test coverage, I updated a couple of test scenarios in dynamic_multi_master-test.cc to provide initial coverage. In addition, a follow-up patch will add a new test scenario into flex_partitioning_client-test.cc. Change-Id: Id9805dc3fb4ba0a734ca92198bc5dc6449f588b7 Reviewed-on: http://gerrit.cloudera.org:8080/18631 Tested-by: Kudu Jenkins Reviewed-by: Attila Bukor <abu...@apache.org> --- src/kudu/client/master_proxy_rpc.cc | 11 +++++++++-- src/kudu/client/txn_manager_proxy_rpc.cc | 16 ++++++++++------ src/kudu/master/dynamic_multi_master-test.cc | 9 ++++++--- src/kudu/tserver/heartbeater.cc | 10 +++++++++- 4 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/kudu/client/master_proxy_rpc.cc b/src/kudu/client/master_proxy_rpc.cc index da4bb1ed4..8b8a2b814 100644 --- a/src/kudu/client/master_proxy_rpc.cc +++ b/src/kudu/client/master_proxy_rpc.cc @@ -28,6 +28,7 @@ #include "kudu/client/client-internal.h" #include "kudu/client/client.h" #include "kudu/common/wire_protocol.h" +#include "kudu/gutil/strings/join.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/master/master.pb.h" #include "kudu/rpc/response_callback.h" @@ -77,6 +78,7 @@ using master::ListTabletServersRequestPB; using master::ListTabletServersResponsePB; using master::MasterServiceProxy; using master::MasterErrorPB; +using master::MasterFeatures_Name; using master::RemoveMasterRequestPB; using master::RemoveMasterResponsePB; using master::ReplaceTabletRequestPB; @@ -259,8 +261,13 @@ bool AsyncLeaderMasterRpc<ReqClass, RespClass>::RetryOrReconnectIfNecessary( return true; } if (err->unsupported_feature_flags_size() > 0) { - s = Status::NotSupported(Substitute("Cluster does not support $0", - rpc_name_)); + const auto features_str = JoinMapped(err->unsupported_feature_flags(), + [](uint32_t feature) { + return MasterFeatures_Name(feature); + }, ","); + s = Status::NotSupported( + Substitute("cluster does not support $0 with feature(s) $1", + rpc_name_, features_str)); } } diff --git a/src/kudu/client/txn_manager_proxy_rpc.cc b/src/kudu/client/txn_manager_proxy_rpc.cc index ac7c03f7e..42a4bc59d 100644 --- a/src/kudu/client/txn_manager_proxy_rpc.cc +++ b/src/kudu/client/txn_manager_proxy_rpc.cc @@ -18,6 +18,7 @@ #include "kudu/client/txn_manager_proxy_rpc.h" #include <algorithm> +#include <cstdint> #include <functional> #include <memory> #include <ostream> @@ -28,6 +29,7 @@ #include "kudu/client/client-internal.h" #include "kudu/client/client.h" #include "kudu/common/wire_protocol.h" +#include "kudu/gutil/strings/join.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/master/txn_manager.pb.h" #include "kudu/rpc/response_callback.h" @@ -58,6 +60,7 @@ using kudu::transactions::KeepTransactionAliveRequestPB; using kudu::transactions::KeepTransactionAliveResponsePB; using kudu::transactions::TxnManagerServiceProxy; using kudu::transactions::TxnManagerErrorPB; +using kudu::transactions::TxnManagerFeatures_Name; using std::string; using strings::Substitute; @@ -207,13 +210,14 @@ bool AsyncRandomTxnManagerRpc<ReqClass, RespClass>::RetryIfNecessary( } return true; } - // TODO(aserbin): report unsupported features in the error message if it - // starts making sense: of course this code is forward - // looking, but it's not clear how the detailed information - // on missing features could help in making this error - // message more actionable if (err->unsupported_feature_flags_size() > 0) { - s = Status::NotSupported("TxnManager is missing required features"); + const auto required_features_str = JoinMapped( + err->unsupported_feature_flags(), + [](uint32_t feature) { + return TxnManagerFeatures_Name(feature); + }, ","); + s = Status::NotSupported("TxnManager is missing required feature(s): $0", + required_features_str); } } diff --git a/src/kudu/master/dynamic_multi_master-test.cc b/src/kudu/master/dynamic_multi_master-test.cc index 29d89ca99..9bf06f8cc 100644 --- a/src/kudu/master/dynamic_multi_master-test.cc +++ b/src/kudu/master/dynamic_multi_master-test.cc @@ -1165,7 +1165,8 @@ TEST_F(DynamicMultiMasterTest, TestAddMasterFeatureFlagNotSpecified) { ASSERT_OK(BuildMasterOpts(orig_num_masters_, reserved_hp_, &opts)); Status actual = AddMasterToClusterUsingCLITool(opts, &err); ASSERT_TRUE(actual.IsRuntimeError()) << actual.ToString(); - ASSERT_STR_MATCHES(err, "Cluster does not support AddMaster"); + ASSERT_STR_CONTAINS(err, "cluster does not support AddMaster " + "with feature(s) DYNAMIC_MULTI_MASTER"); // Verify no change in number of masters. NO_FATALS(VerifyVoterMasters(orig_num_masters_)); @@ -1189,7 +1190,8 @@ TEST_F(DynamicMultiMasterTest, TestRemoveMasterFeatureFlagNotSpecified) { string err; Status s = RemoveMasterFromClusterUsingCLITool(master_to_remove, &err); ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); - ASSERT_STR_MATCHES(err, "Cluster does not support RemoveMaster"); + ASSERT_STR_CONTAINS(err, "cluster does not support RemoveMaster " + "with feature(s) DYNAMIC_MULTI_MASTER"); } // Try removing leader master @@ -1200,7 +1202,8 @@ TEST_F(DynamicMultiMasterTest, TestRemoveMasterFeatureFlagNotSpecified) { string err; Status s = RemoveMasterFromClusterUsingCLITool(master_to_remove, &err); ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); - ASSERT_STR_MATCHES(err, "Cluster does not support RemoveMaster"); + ASSERT_STR_CONTAINS(err, "cluster does not support RemoveMaster " + "with feature(s) DYNAMIC_MULTI_MASTER"); } // Verify no change in number of masters. diff --git a/src/kudu/tserver/heartbeater.cc b/src/kudu/tserver/heartbeater.cc index 7126cc539..7b92cbc15 100644 --- a/src/kudu/tserver/heartbeater.cc +++ b/src/kudu/tserver/heartbeater.cc @@ -42,6 +42,7 @@ #include "kudu/gutil/map-util.h" #include "kudu/gutil/port.h" #include "kudu/gutil/ref_counted.h" +#include "kudu/gutil/strings/join.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/master/master.pb.h" #include "kudu/master/master.proxy.h" @@ -106,6 +107,7 @@ TAG_FLAG(heartbeat_inject_required_feature_flag, unsafe); DECLARE_bool(raft_prepare_replacement_before_eviction); using kudu::consensus::ReplicaManagementInfoPB; +using kudu::master::MasterFeatures_Name; using kudu::master::MasterErrorPB; using kudu::master::MasterFeatures; using kudu::master::MasterServiceProxy; @@ -615,7 +617,13 @@ void Heartbeater::Thread::RunThread() { msg = Substitute("master detected incompatibility: $0", err_msg); } if (s.IsRemoteError() && error_status.unsupported_feature_flags_size() > 0) { - msg = Substitute("master does not support required feature flags: $0", err_msg); + const auto required_features_str = JoinMapped( + error_status.unsupported_feature_flags(), + [](uint32_t feature) { + return MasterFeatures_Name(feature); + }, ","); + msg = Substitute("master does not support required feature flag(s) $0: $1", + required_features_str, err_msg); } if (!msg.empty()) { if (FLAGS_heartbeat_incompatible_replica_management_is_fatal) {