You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2022/06/17 18:24:48 UTC
[kudu] branch master updated: [rpc] report the names of the unsupported feature flags
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 <al...@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 <ab...@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) {