Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13632 )
Change subject: KUDU-2823 Place tablet replicas based on dimension ...................................................................... Patch Set 18: (18 comments) http://gerrit.cloudera.org:8080/#/c/13632/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13632/18//COMMIT_MSG@7 PS18, Line 7: Place tablet replicas based on dimension BTW, what would you expect from the rebalancer with regard to balancing the distribution of the replicas in each dimension? http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/client/client.h@1215 PS18, Line 1215: /// @param [in] dimension_label : /// The dimension label for the tablet to be created. Defaults to empty. remove http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/client/client.h@1238 PS18, Line 1238: /// @note By default (or if the cluster is configured without : /// '--master_place_tablet_replicas_based_on_dimension'), the master will try to place : /// newly created tablet replicas on tablet servers with a small number of tablet replicas. : /// If the dimension label is provided, newly created replicas will be evenly distributed : /// in the cluster based on the dimension label. In other words, the master will try to place : /// newly created tablet replicas on tablet servers with a small number of tablet replicas : /// belonging to this dimension label. Could you replace this by: See KuduTableCreator::dimension_label() for details on dimension label. http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@146 PS18, Line 146: tablet nit: the tablet http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@146 PS18, Line 146: Used by the master to determine load when : // creating new tablet replicas based on dimension. How about: Used for dimension-specific replica placement. http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@368 PS18, Line 368: NOTE: If the heartbeat request wants to send 'num_live_tablets_by_dimension', then the tablet : // server needs to count the number of tablet replicas in each dimension, which requires : // traversing all the tablet replicas in the tablet server. This is just an implementation detail. The information could be kept in run-time structures, so there would be no need to traverse and re-evaluate that again and again each heartbeat. Also, currently each tablet servers traverses its registry for finding the total number of alive and bootstrapping tablet replicas anyways. That might be a small optimization piece (I think it's better to do that in a separate patch :) ) http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@374 PS18, Line 374: optional bool needs_num_live_tablets_by_dimension = 10 [ default = false ]; Adar has already pointed at this in prior review iterations, and I tend agree with him here. IMO, there is not much value in introducing this field once there is explicit specification of dimension labels for tables and extra tablets (partitions). What if we drop both this field and also the --master_place_tablet_replicas_based_on_dimension master's flag? One additional argument to drop these is to think how we should report on the status of tables' creation and adding extra partitions to clients in case of they specify dimension labels when master is run with --master_place_tablet_replicas_based_on_dimension=false ? http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@482 PS18, Line 482: Used by the master to determine load when creating new tablet replicas based on dimension. Maybe: Used for dimension-specific placement of tablet replicas corresponding to the partitions of the newly created table. http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@614 PS18, Line 614: tablet the corresponding tablet http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@614 PS18, Line 614: Used by the master to determine load when : // creating new tablet replicas based on dimension. How about: Used for dimension-specific placement of the tablet's replicas. http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master_service.cc@255 PS18, Line 255: FLAGS_master_place_tablet_replicas_based_on_dimension Do you think we still need this even if the dimension labels are specified explicitly? http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc File src/kudu/master/placement_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@261 PS18, Line 261: { {} } nit: I think this is not needed? It will be initialized as empty if omitted, I think. http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@291 PS18, Line 291: { : TSDescriptorVector existing(all.begin(), all.end()); : existing.pop_back(); : existing.pop_back(); : shared_ptr<TSDescriptor> extra_ts; : ASSERT_OK(policy.PlaceExtraTabletReplica(existing, boost::none, &extra_ts)); : ASSERT_TRUE(extra_ts); : ASSERT_EQ("ts2", extra_ts->permanent_uuid()); : } : : { : TSDescriptorVector existing(all.begin(), all.end()); : existing.pop_back(); : existing.pop_back(); : shared_ptr<TSDescriptor> extra_ts; : ASSERT_OK(policy.PlaceExtraTabletReplica( : existing, boost::make_optional(string("labelA")), &extra_ts)); : ASSERT_TRUE(extra_ts); : ASSERT_EQ("ts2", extra_ts->permanent_uuid()); : } For the purpose of readability and easier comprehension, could you put the similar sub-scopes in this scenario under 'for' loop, like: for (const auto& label : { boost::none, boost::make_optional(string("labelA") }) { ... } ? http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@341 PS18, Line 341: { {} } nit: drop http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@365 PS18, Line 365: // Ask for number of replicas equal to the number of available tablet servers. : { : TSDescriptorVector result; : ASSERT_OK(policy.PlaceTabletReplicas(3, boost::none, &result)); : ASSERT_EQ(3, result.size()); : TSDescriptorsMap m; : ASSERT_OK(TSDescriptorVectorToMap(result, &m)); : ASSERT_EQ(1, m.count("ts0")); : ASSERT_EQ(1, m.count("ts1")); : ASSERT_EQ(1, m.count("ts2")); : } : : // Ask for number of replicas with 'labelA' equal to the number of available tablet servers. : { : TSDescriptorVector result; : ASSERT_OK(policy.PlaceTabletReplicas(3, boost::make_optional(string("labelA")), &result)); : ASSERT_EQ(3, result.size()); : TSDescriptorsMap m; : ASSERT_OK(TSDescriptorVectorToMap(result, &m)); : ASSERT_EQ(1, m.count("ts0")); : ASSERT_EQ(1, m.count("ts1")); : ASSERT_EQ(1, m.count("ts2")); : } Put this under for() {} scope as well? http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@389 PS18, Line 389: // Try to ask for too many replicas when too few tablet servers are available. : { : TSDescriptorVector result; : auto s = policy.PlaceTabletReplicas(4, boost::none, &result); : ASSERT_TRUE(s.IsNotFound()) << s.ToString(); : ASSERT_STR_CONTAINS(s.ToString(), : "could not find next location after placing " : "3 out of 4 tablet replicas"); : ASSERT_TRUE(result.empty()); : } : : // Try to ask for too many replicas with 'labelA' when too few tablet servers are available. : { : TSDescriptorVector result; : auto s = policy.PlaceTabletReplicas(4, boost::make_optional(string("labelA")), &result); : ASSERT_TRUE(s.IsNotFound()) << s.ToString(); : ASSERT_STR_CONTAINS(s.ToString(), : "could not find next location after placing " : "3 out of 4 tablet replicas"); : ASSERT_TRUE(result.empty()); ditto http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@963 PS18, Line 963: // When the cluster has new tablet servers, comparison test two policy: : // 1. Get the number of tablet replicas on tablet server. : // 2. Get the number of tablet replicas in the specified dimension on tablet server. How about: In a Kudu cluster with newly added tablet servers, add tablet replicas with and without dimension label and verify the result distribution of the replicas. Check for 1) overall distribution of replicas 2) distribution of replicas within the specified dimension. http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@971 PS18, Line 971: { {} } nit: drop -- 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: 18 Gerrit-Owner: Yao Xu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Comment-Date: Sat, 13 Jul 2019 03:03:48 +0000 Gerrit-HasComments: Yes
