ZhangYao has posted comments on this change. ( http://gerrit.cloudera.org:8080/14608 )
Change subject: KUDU-2987 Intra location rebalance crashes in special case. ...................................................................... Patch Set 4: (23 comments) http://gerrit.cloudera.org:8080/#/c/14608/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14608/3//COMMIT_MSG@7 PS3, Line 7: crashes in > crashes Done http://gerrit.cloudera.org:8080/#/c/14608/3//COMMIT_MSG@8 PS3, Line 8: > Maybe, describe what were the conditions which lead to the crash scenario. Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc File src/kudu/tools/rebalancer_tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@483 PS3, Line 483: // CreateUnbalancedTables() but the set of tablet servers to avoid is defined > Document the newly introduced parameter 'table_name_pattern': it' not obvio Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@487 PS3, Line 487: // function. Each call to this function can create differen > code style: input parameters should come before output parameters in the fu Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@523 PS3, Line 523: > elsewhere but not Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@524 PS3, Line 524: : return Status:: > Drop. Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@526 PS3, Line 526: > Tservers Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@531 PS3, Line 531: ed_map<string, int>& > nit: excluded_tserver_uuids Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@537 PS3, Line 537: cas would be hosted by those servers. > Is it possible to use ContainsKey() here instead? Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@545 PS3, Line 545: cluster_->tablet_server_by_uuid(ts->uuid())->Shutd > Drop: this sentence isn't adding any clarity and from the code it's obvious Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@547 PS3, Line 547: > nit: the expected value comes first (i.e. swap these two) Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1716 PS3, Line 1716: class IntraLocationRebalancingBasicTest : public RebalancingTest { : public: : I > nit: fix the alignment of these two lines Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1725 PS3, Line 1725: > nit: I think it's better to name this scenario to reflect the essence of wh Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1733 PS3, Line 1733: } > nit: add space Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1746 PS3, Line 1746: FLAGS_num_replicas = rep_factor_; > I think it's important to note that there are 3 tablets in each table, so t Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1747 PS3, Line 1747: _, mast > tablet replicas Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1749 PS3, Line 1749: > table0 with tablet replicas in locations /A, /B, and /C. Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1750 PS3, Line 1750: cial intra loc > is not greater than 1 Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1750 PS3, Line 1750: : // should only contain t > no replica of table0 will be moved Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1764 PS3, Line 1764: : const vector<st > Drop since this is a bit confusing -- the rebalancer actually moves one rep Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1774 PS3, Line 1774: > rebalancing Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1774 PS3, Line 1774: > should move Done http://gerrit.cloudera.org:8080/#/c/14608/3/src/kudu/tools/rebalancer_tool-test.cc@1777 PS3, Line 1777: ASSERT_TRUE(s.ok()) << ToolRunInfo(s, out, err); : ASSERT_STR_CO > There were no placement policy violations ever prior to running the rebalan Yeah. I moved this comment, it's confusing and not suitable here. -- To view, visit http://gerrit.cloudera.org:8080/14608 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea39472e55178fca688b390249432a0f7fefaaba Gerrit-Change-Number: 14608 Gerrit-PatchSet: 4 Gerrit-Owner: ZhangYao <triplesheep0...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: ZhangYao <triplesheep0...@gmail.com> Gerrit-Comment-Date: Fri, 08 Nov 2019 03:44:20 +0000 Gerrit-HasComments: Yes
