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 2025/04/08 21:31:06 UTC
(kudu) 03/05: [master] fix result status handling
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch branch-1.18.x
in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 5df4e06dfdb5861aec9545818572c3294088df6a
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Mon Mar 31 14:37:47 2025 -0700
[master] fix result status handling
This patch addresses sloppy result handling of functions/methods
that return Status under the src/kudu/master directory.
Change-Id: Id6c165b6db7ddfb8a838d5c92a029349a6bb21be
Reviewed-on: http://gerrit.cloudera.org:8080/22717
Tested-by: Kudu Jenkins
Reviewed-by: Zoltan Chovan <zc...@cloudera.com>
Reviewed-by: Marton Greber <gr...@gmail.com>
(cherry picked from commit 97df171f849a10ecbc3613962440f5d84b18314a)
Reviewed-on: http://gerrit.cloudera.org:8080/22759
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
Tested-by: Alexey Serbin <al...@apache.org>
---
src/kudu/master/auto_leader_rebalancer-test.cc | 64 ++++++++++++++------------
src/kudu/master/auto_leader_rebalancer.cc | 3 +-
src/kudu/master/auto_rebalancer.cc | 2 +-
src/kudu/master/catalog_manager.cc | 10 ++--
src/kudu/master/master-test.cc | 3 +-
src/kudu/master/sys_catalog-test.cc | 2 +-
6 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/src/kudu/master/auto_leader_rebalancer-test.cc b/src/kudu/master/auto_leader_rebalancer-test.cc
index d9f5949c0..dd59c612a 100644
--- a/src/kudu/master/auto_leader_rebalancer-test.cc
+++ b/src/kudu/master/auto_leader_rebalancer-test.cc
@@ -36,6 +36,7 @@
#include "kudu/consensus/consensus.pb.h"
#include "kudu/consensus/metadata.pb.h"
#include "kudu/gutil/ref_counted.h"
+#include "kudu/gutil/strings/substitute.h"
#include "kudu/integration-tests/test_workload.h"
#include "kudu/master/catalog_manager.h"
#include "kudu/master/master.h"
@@ -143,23 +144,23 @@ class LeaderRebalancerTest : public KuduTest {
}
// Get the leader numbers of each tablet server.
- void GetLeaderDistribution(std::map<string, int32_t>* leader_map,
+ Status GetLeaderDistribution(std::map<string, int32_t>* leader_map,
const string& table_name) {
leader_map->clear();
shared_ptr<KuduTable> table;
- workload_->client()->OpenTable(table_name, &table);
+ RETURN_NOT_OK(workload_->client()->OpenTable(table_name, &table));
scoped_refptr<TableInfo> table_info;
master::Master* master = cluster_->mini_master()->master();
master::CatalogManager* catalog_manager = master->catalog_manager();
{
CatalogManager::ScopedLeaderSharedLock leaderlock(catalog_manager);
- catalog_manager->GetTableInfo(table->id(), &table_info);
+ RETURN_NOT_OK(catalog_manager->GetTableInfo(table->id(), &table_info));
}
std::vector<string> leader_list;
for (const auto& tablet : table_info->tablet_map()) {
client::KuduTablet* ptr;
- workload_->client()->GetTablet(tablet.second->id(), &ptr);
+ RETURN_NOT_OK(workload_->client()->GetTablet(tablet.second->id(), &ptr));
unique_ptr<client::KuduTablet> tablet_ptr(ptr);
for (const auto* replica : tablet_ptr->replicas()) {
if (replica->is_leader()) {
@@ -176,6 +177,8 @@ class LeaderRebalancerTest : public KuduTest {
leader_map->emplace(e->permanent_uuid(), count(
leader_list.begin(), leader_list.end(), e->permanent_uuid()));
}
+
+ return Status::OK();
}
// Make the leader distribution as the vector passed in.
@@ -214,7 +217,7 @@ class LeaderRebalancerTest : public KuduTest {
unique_ptr<client::KuduTablet> tablet_copy;
{
client::KuduTablet* ptr;
- workload_->client()->GetTablet(tablet.second->id(), &ptr);
+ RETURN_NOT_OK(workload_->client()->GetTablet(tablet.second->id(), &ptr));
tablet_copy.reset(ptr);
}
for (const auto* replica: tablet_copy->replicas()) {
@@ -261,11 +264,11 @@ TEST_F(LeaderRebalancerTest, FunctionalTestForDivided) {
// Simulate the leader distribution.
std::vector<int32_t> leader_distribution = {4, 4, 1};
- MakeLeaderDistribution(leader_distribution, table_name());
+ ASSERT_OK(MakeLeaderDistribution(leader_distribution, table_name()));
SleepFor(MonoDelta::FromMilliseconds(3000));
std::map<string, int32_t> leader_map;
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
@@ -277,14 +280,14 @@ TEST_F(LeaderRebalancerTest, FunctionalTestForDivided) {
master::AutoLeaderRebalancerTask* leader_rebalancer =
master->catalog_manager()->auto_leader_rebalancer();
for (int i = 0; i < retries; i++) {
- leader_rebalancer->RunLeaderRebalancer();
+ ASSERT_OK(leader_rebalancer->RunLeaderRebalancer());
SleepFor(MonoDelta::FromMilliseconds(FLAGS_heartbeat_interval_ms));
}
// Check the leader numbers of each tablet server. It should always be floor(avg)
// or ceil(avg), where the parameter avg is (tablet num) / (tablet server num).
double expected_leader_num = static_cast<double>(kNumTablets) / 3;
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
@@ -296,21 +299,22 @@ TEST_F(LeaderRebalancerTest, FunctionalTestForDivided) {
// Try different leader distribution.
std::vector<int32_t> leader_distribution2 = {0, 8, 1};
- MakeLeaderDistribution(leader_distribution2, table_name());
+ ASSERT_OK(MakeLeaderDistribution(leader_distribution2, table_name()));
SleepFor(MonoDelta::FromMilliseconds(3000));
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
}
for (int i = 0; i < retries; i++) {
- leader_rebalancer->RunLeaderRebalancer();
+ SCOPED_TRACE(strings::Substitute("try $0", i));
+ ASSERT_OK(leader_rebalancer->RunLeaderRebalancer());
SleepFor(MonoDelta::FromMilliseconds(FLAGS_heartbeat_interval_ms));
}
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
@@ -331,11 +335,11 @@ TEST_F(LeaderRebalancerTest, FunctionalTestForNotDivided) {
// Simulate the leader distribution.
std::vector<int32_t> leader_distribution = {5, 4, 1};
- MakeLeaderDistribution(leader_distribution, table_name());
+ ASSERT_OK(MakeLeaderDistribution(leader_distribution, table_name()));
SleepFor(MonoDelta::FromMilliseconds(3000));
std::map<string, int32_t> leader_map;
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
@@ -347,14 +351,14 @@ TEST_F(LeaderRebalancerTest, FunctionalTestForNotDivided) {
master::AutoLeaderRebalancerTask* leader_rebalancer =
master->catalog_manager()->auto_leader_rebalancer();
for (int i = 0; i < retries; i++) {
- leader_rebalancer->RunLeaderRebalancer();
+ ASSERT_OK(leader_rebalancer->RunLeaderRebalancer());
SleepFor(MonoDelta::FromMilliseconds(FLAGS_heartbeat_interval_ms));
}
// Check the leader numbers of each tablet server. It should always be floor(avg)
// or ceil(avg), where the parameter avg is (tablet num) / (tablet server num).
double expected_leader_num = static_cast<double>(kNumTablets) / 3;
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
@@ -366,21 +370,21 @@ TEST_F(LeaderRebalancerTest, FunctionalTestForNotDivided) {
// Try different leader distribution.
std::vector<int32_t> leader_distribution2 = {8, 1, 1};
- MakeLeaderDistribution(leader_distribution2, table_name());
+ ASSERT_OK(MakeLeaderDistribution(leader_distribution2, table_name()));
SleepFor(MonoDelta::FromMilliseconds(3000));
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
}
for (int i = 0; i < retries; i++) {
- leader_rebalancer->RunLeaderRebalancer();
+ ASSERT_OK(leader_rebalancer->RunLeaderRebalancer());
SleepFor(MonoDelta::FromMilliseconds(FLAGS_heartbeat_interval_ms));
}
- GetLeaderDistribution(&leader_map, table_name());
+ ASSERT_OK(GetLeaderDistribution(&leader_map, table_name()));
LOG(INFO) << "The leader distribution is " << '\n';
for (const auto& leader : leader_map) {
std::cout << leader.first << " " << leader.second << '\n';
@@ -425,7 +429,7 @@ TEST_F(LeaderRebalancerTest, AddTserver) {
SleepFor(MonoDelta::FromSeconds(20 * FLAGS_auto_rebalancing_interval_seconds));
constexpr const int32_t retries = 40;
for (int i = 0; i < retries; i++) {
- leader_rebalancer->RunLeaderRebalancer();
+ ASSERT_OK(leader_rebalancer->RunLeaderRebalancer());
if (CheckLeaderBalance().ok()) {
break;
}
@@ -453,12 +457,12 @@ TEST_F(LeaderRebalancerTest, RestartTserver) {
ASSERT_NE(replica_rebalancer, nullptr);
ASSERT_NE(leader_rebalancer, nullptr);
- cluster_->mini_tablet_server(0)->Restart();
+ ASSERT_OK(cluster_->mini_tablet_server(0)->Restart());
// To wait replica_rebalancer execute some runs and reach balanced.
SleepFor(MonoDelta::FromSeconds(10 * FLAGS_auto_rebalancing_interval_seconds));
constexpr const int32_t retries = 20;
for (int i = 0; i < retries; i++) {
- leader_rebalancer->RunLeaderRebalancer();
+ ASSERT_OK(leader_rebalancer->RunLeaderRebalancer());
if (CheckLeaderBalance().ok()) {
break;
}
@@ -553,7 +557,7 @@ TEST_P(FilterSoftDeletedTableTest, TestFilterSofteDeletedTable) {
// Simulate the leader distribution.
std::vector<int32_t> leader_distribution = {4, 4, 1};
- MakeLeaderDistribution(leader_distribution, table_name());
+ ASSERT_OK(MakeLeaderDistribution(leader_distribution, table_name()));
SleepFor(MonoDelta::FromMilliseconds(3000));
string first_table = table_name();
@@ -566,11 +570,11 @@ TEST_P(FilterSoftDeletedTableTest, TestFilterSofteDeletedTable) {
workload_->Setup();
// Simulate the leader distribution.
- MakeLeaderDistribution(leader_distribution, soft_deleted_table);
+ ASSERT_OK(MakeLeaderDistribution(leader_distribution, soft_deleted_table));
SleepFor(MonoDelta::FromMilliseconds(3000));
// Delete the table 'soft_deleted_table'.
- workload_->client()->SoftDeleteTable(soft_deleted_table, 3600);
+ ASSERT_OK(workload_->client()->SoftDeleteTable(soft_deleted_table, 3600));
// Try to run leader rebalance for 10 times.
int32_t retries = 10;
@@ -578,18 +582,18 @@ TEST_P(FilterSoftDeletedTableTest, TestFilterSofteDeletedTable) {
master::AutoLeaderRebalancerTask* leader_rebalancer =
master->catalog_manager()->auto_leader_rebalancer();
for (int i = 0; i < retries; i++) {
- leader_rebalancer->RunLeaderRebalancer();
+ ASSERT_OK(leader_rebalancer->RunLeaderRebalancer());
SleepFor(MonoDelta::FromMilliseconds(FLAGS_heartbeat_interval_ms));
}
std::map<string, int32_t> leader_map;
// The first table is leader rebalanced.
- GetLeaderDistribution(&leader_map, first_table);
+ ASSERT_OK(GetLeaderDistribution(&leader_map, first_table));
for (const auto& leader: leader_map) {
ASSERT_EQ(leader.second, 3);
}
- GetLeaderDistribution(&leader_map, soft_deleted_table);
+ ASSERT_OK(GetLeaderDistribution(&leader_map, soft_deleted_table));
// The soft deleted table is not leader rebalanced.
if (FLAGS_leader_rebalancing_ignore_soft_deleted_tables) {
for (const auto& leader: leader_map) {
diff --git a/src/kudu/master/auto_leader_rebalancer.cc b/src/kudu/master/auto_leader_rebalancer.cc
index 6d2c6091f..0398b7574 100644
--- a/src/kudu/master/auto_leader_rebalancer.cc
+++ b/src/kudu/master/auto_leader_rebalancer.cc
@@ -443,7 +443,8 @@ Status AutoLeaderRebalancerTask::RunLeaderRebalancer() {
}
}
for (const auto& table_info : table_infos) {
- RunLeaderRebalanceForTable(table_info, tserver_uuids, exclude_dest_uuids);
+ RETURN_NOT_OK(RunLeaderRebalanceForTable(
+ table_info, tserver_uuids, exclude_dest_uuids));
}
// @TODO(duyuqi)
// Enrich the log and add metrics for leader rebalancer.
diff --git a/src/kudu/master/auto_rebalancer.cc b/src/kudu/master/auto_rebalancer.cc
index 518c3831e..39807905b 100644
--- a/src/kudu/master/auto_rebalancer.cc
+++ b/src/kudu/master/auto_rebalancer.cc
@@ -317,7 +317,7 @@ Status AutoRebalancerTask::GetMoves(
for (const auto& elem : ts_id_by_location) {
const auto& location = elem.first;
ClusterRawInfo location_raw_info;
- BuildClusterRawInfo(location, &location_raw_info);
+ RETURN_NOT_OK(BuildClusterRawInfo(location, &location_raw_info));
RETURN_NOT_OK(GetMovesUsingRebalancingAlgo(
location_raw_info, &algo, CrossLocations::NO, &rep_moves));
}
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index ef9f3827a..1a2d15438 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -921,10 +921,10 @@ void CheckIfNoLongerLeaderAndSetupError(const Status& s, RespClass* resp) {
// that is no longer the leader, this suffices until we
// distinguish this cause of write failure more explicitly.
if (s.IsIllegalState() || s.IsAborted()) {
- SetupError(Status::ServiceUnavailable(
- "operation requested can only be executed on a leader master, but this"
- " master is no longer the leader", s.ToString()),
- resp, MasterErrorPB::NOT_THE_LEADER);
+ ignore_result(SetupError(Status::ServiceUnavailable(
+ "operation requested can only be executed on a leader master, "
+ "but this master is no longer the leader", s.ToString()),
+ resp, MasterErrorPB::NOT_THE_LEADER));
}
}
@@ -3163,7 +3163,7 @@ Status CatalogManager::ApplyAlterPartitioningSteps(
RETURN_NOT_OK(partition_schema.DropRange(
range_bound.first, range_bound.second, schema));
PartitionSchemaPB ps_pb;
- partition_schema.ToPB(schema, &ps_pb);
+ RETURN_NOT_OK(partition_schema.ToPB(schema, &ps_pb));
// Make sure exactly one range is gone.
DCHECK_EQ(ps_pb.custom_hash_schema_ranges_size() + 1,
l->data().pb.partition_schema().custom_hash_schema_ranges_size());
diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc
index a87dbc6e9..28f162a80 100644
--- a/src/kudu/master/master-test.cc
+++ b/src/kudu/master/master-test.cc
@@ -54,6 +54,7 @@
#include "kudu/consensus/consensus.pb.h"
#include "kudu/consensus/replica_management.pb.h"
#include "kudu/generated/version_defines.h"
+#include "kudu/gutil/basictypes.h"
#include "kudu/gutil/dynamic_annotations.h"
#include "kudu/gutil/integral_types.h"
#include "kudu/gutil/map-util.h"
@@ -2203,7 +2204,7 @@ TEST_F(MasterTest, TestGetTableLocationsDuringRepeatedTableVisit) {
// Call ElectedAsLeaderCb() repeatedly. If these spurious calls aren't
// ignored, the concurrent GetTableLocations() calls may crash the master.
for (int i = 0; i < 100; i++) {
- master_->catalog_manager()->ElectedAsLeaderCb();
+ ignore_result(master_->catalog_manager()->ElectedAsLeaderCb());
}
done.Store(true);
t.join();
diff --git a/src/kudu/master/sys_catalog-test.cc b/src/kudu/master/sys_catalog-test.cc
index 8a0a940f2..35e150305 100644
--- a/src/kudu/master/sys_catalog-test.cc
+++ b/src/kudu/master/sys_catalog-test.cc
@@ -471,7 +471,7 @@ TEST_F(SysCatalogTest, LoadClusterID) {
// Restart the master and ensure the generated cluster ID is the same.
mini_master_->Shutdown();
- mini_master_->Restart();
+ ASSERT_OK(mini_master_->Restart());
ASSERT_OK(mini_master_->master()->catalog_manager()->sys_catalog()->
GetClusterIdEntry(&cluster_id_entry));
ASSERT_EQ(init_id, cluster_id_entry.cluster_id());