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 <[email protected]> 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 <[email protected]> Reviewed-by: Marton Greber <[email protected]> (cherry picked from commit 97df171f849a10ecbc3613962440f5d84b18314a) Reviewed-on: http://gerrit.cloudera.org:8080/22759 Reviewed-by: Abhishek Chennaka <[email protected]> Tested-by: Alexey Serbin <[email protected]> --- 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());
