Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13632 )
Change subject: KUDU-2823 Place tablet replicas based on range ...................................................................... Patch Set 7: (15 comments) http://gerrit.cloudera.org:8080/#/c/13632/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13632/7//COMMIT_MSG@16 PS7, Line 16: Unfortunately, after we add some new tablet servers to the cluster, : creating a new tablet replica will prioritize the new tablet server for : placement according to the current placement policy Not related to this patch as is, but I'm curious: would running the rebalancer after adding a new tablet servers into the system but before creating new partitions could help? http://gerrit.cloudera.org:8080/#/c/13632/7//COMMIT_MSG@23 PS7, Line 23: So, I added a new placement policy. When creating a new tablet replica, : we prefer to select tablet server which a smaller number of tablet : replicas in the range Is it possible to generalize this policy, making it as an additional dimension to the number of replicas at a tablet server? That way we could make it a good default policy for placing new replicas. http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/integration-tests/create-table-itest.cc File src/kudu/integration-tests/create-table-itest.cc: http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/integration-tests/create-table-itest.cc@223 PS7, Line 223: hot 'hot' or 'cold' depends on the use case. As I understand, this test verifies that the replicas of the newly created tablets are placed to distribute replicas of the tablets in the same range evenly among tablet servers. http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/catalog_manager.cc@227 PS7, Line 227: place places http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/catalog_manager.cc@228 PS7, Line 228: the number tablets in a " : "tablet server the number of tablets at a tablet server http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/catalog_manager.cc@3741 PS7, Line 3741: if (PREDICT_FALSE(!s.ok())) { : auto msg = Substitute("no extra replica candidate found for tablet $0: $1", : tablet_->ToString(), s.ToString()); Is this still valid if the new code at L3725 returns non-OK status? http://gerrit.cloudera.org:8080/#/c/13632/4/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13632/4/src/kudu/master/master.proto@317 PS4, Line 317: when nit: drop http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/placement_policy.h File src/kudu/master/placement_policy.h: http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/placement_policy.h@78 PS7, Line 78: the number tablets in a tablet server the number of tablets at a tablet server http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/placement_policy.h@77 PS7, Line 77: place tablet replicas based on the number of tablets in a : // range That sounds a bit vague to me. What about: place tablet replicas of the same range evenly among tablet servers http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/placement_policy.h@91 PS7, Line 91: place tablet replicas based on the number of tablets in a : // range ditto http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/placement_policy.h@92 PS7, Line 92: the number tablets in a tablet server the number of tables at a tablet server http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/ts_descriptor.h File src/kudu/master/ts_descriptor.h: http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/master/ts_descriptor.h@61 PS7, Line 61: map Why not unordered_map? Is there any significance in the order of the keys in the dictionary? The unordered_map is better if there are many lookup operations on the dictionary. http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/tserver/ts_tablet_manager.cc@1247 PS7, Line 1247: CHECK nit: DCHECK() so it's checked only in debug builds In s release build, it will crash at line 1266 anyways with SIGSEGV. http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/tserver/ts_tablet_manager.cc@1250 PS7, Line 1250: const auto &entry style nit: const auto& entry http://gerrit.cloudera.org:8080/#/c/13632/7/src/kudu/tserver/ts_tablet_manager.cc@1260 PS7, Line 1260: auto* existing = InsertOrReturnExisting(&result, range, 1); : if (existing) { : (*existing)++; : } nit: consider using LookupOrEmplace() for more clarity since it's not important in this context whether the element was there or not: auto& ranges = LookupOrEmplace(&result, range, 0); ++ranges; -- To view, visit http://gerrit.cloudera.org:8080/13632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48a225e221eb42ef2f5489687e80a151d8dc1a42 Gerrit-Change-Number: 13632 Gerrit-PatchSet: 7 Gerrit-Owner: Yao Xu <oclarms....@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Yao Xu <oclarms....@gmail.com> Gerrit-Comment-Date: Mon, 17 Jun 2019 14:45:45 +0000 Gerrit-HasComments: Yes
