You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/03/06 06:21:07 UTC
[kudu-CR] WIP [tests] TS registration flood test
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12679
Change subject: WIP [tests] TS registration flood test
......................................................................
WIP [tests] TS registration flood test
This patch adds a simple test to flood masters with TS
registration heartbeats while masters are running with
location assignment cache disabled.
WIP this test still fails due to overflow of master's RPC queue.
Change-Id: I848cb01a810ca3301ff22e87d23d87260b5ee13f
---
M src/kudu/integration-tests/location_assignment-itest.cc
M src/kudu/master/location_cache.cc
M src/kudu/master/location_cache.h
M src/kudu/master/master_service.cc
M src/kudu/master/ts_descriptor.cc
5 files changed, 137 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/12679/1
--
To view, visit http://gerrit.cloudera.org:8080/12679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I848cb01a810ca3301ff22e87d23d87260b5ee13f
Gerrit-Change-Number: 12679
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP [tests] TS registration flood test
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12679 )
Change subject: WIP [tests] TS registration flood test
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/integration-tests/location_assignment-itest.cc@354
PS1, Line 354: const int TsRegistrationFloodITest::kNumTabletServers = 62;
Would be good to extract this from the EMC framework itself.
Also, after Hao's changes, I'm not sure this is quite right; consider the "partitioning" that goes on in MiniCluster::GetBindIpForDaemonWithType.
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/integration-tests/location_assignment-itest.cc@364
PS1, Line 364: NO_FATALS(cluster_->AssertNoCrashes());
Is this necessary?
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/integration-tests/location_assignment-itest.cc@368
PS1, Line 368: for (auto idx = 0; idx < cluster_->num_masters(); ++idx) {
: ASSERT_OK(cluster_->master(idx)->Pause());
: SleepFor(MonoDelta::FromSeconds(1));
: ASSERT_OK(cluster_->master(idx)->Resume());
: }
: for (auto idx = 0; idx < cluster_->num_masters(); ++idx) {
: cluster_->master(idx)->Shutdown();
: ASSERT_OK(cluster_->master(idx)->Restart());
: }
IIUC, the Pause/Resume cycle is intended to force a leader master election (which triggers a full heartbeat) while the Shutdown/Restart cycle causes the new master to be fresh and request full heartbeats. If full heartbeats are the goal, why do we need both approaches? Wouldn't one suffice?
Also, if you want to trigger leader master elections, GetLeaderMasterIndex() is probably a more effective way to pause (or shutdown) the leader master.
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/integration-tests/location_assignment-itest.cc@378
PS1, Line 378: NO_FATALS(cluster_->AssertNoCrashes());
Nothing about this test suggests that there should be any crashes, so why bother?
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/master/location_cache.h
File src/kudu/master/location_cache.h:
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/master/location_cache.h@56
PS1, Line 56: Status AssignLocation(const std::string& key, std::string* location) const;
Nit: maybe GetLocationNoCache() would be more explicit?
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/12679/1/src/kudu/master/master_service.cc@579
PS1, Line 579: if (PREDICT_TRUE(location_cache != nullptr) &&
Isn't the location cache guaranteed to exist?
--
To view, visit http://gerrit.cloudera.org:8080/12679
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848cb01a810ca3301ff22e87d23d87260b5ee13f
Gerrit-Change-Number: 12679
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 06 Mar 2019 06:59:44 +0000
Gerrit-HasComments: Yes