Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13632 )
Change subject: KUDU-2823 Place tablet replicas based on dimension ...................................................................... Patch Set 22: (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? Emmm, maybe we just need to be able to specify the dimension that we want to balance. 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: KuduPartialRow* lower_bound, : KuduPartialRow* upper_bound, > remove Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/client/client.h@1238 PS18, Line 1238: /// the lower bound is unbounded. If any of the columns are unset, the : /// logical minimum value for the column's type will be used by default. : /// @param [in] upper_bound : /// The upper bound of the range partition to add. If the row is empty, then : /// the upper bound is unbounded. If any of the individual columns are : /// unset, the logical minimum value for the column' type will be used by : /// default. > Could you replace this by: Done 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: the ta > nit: the tablet Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@146 PS18, Line 146: et. Used for dimension-specific : // placement of the tablet's replicas. > How about: Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@368 PS18, Line 368: ///////////////////////// : : message TabletLocationsPB { > This is just an implementation detail. The information could be kept in ru Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@374 PS18, Line 374: required consensus.RaftPeerPB.Role role = 2; > 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 > ? Ok, I'm will drop this field and '--master_place_tablet_replicas_based_on_dimension'. http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@482 PS18, Line 482: > Maybe: Used for dimension-specific placement of tablet replicas correspondi Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@614 PS18, Line 614: > the corresponding tablet Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/master.proto@614 PS18, Line 614: : optional StepType type = 1 [ default = UNKNOWN ]; > How about: Done 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: std::move(TabletNumByDimensionMap(req->num_live_table > Do you think we still need this even if the dimension labels are specified Done 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 omitte Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@291 PS18, Line 291: ASSERT_TRUE(extra_ts); : ASSERT_EQ("ts2", extra_ts->permanent_uuid()); : } : : { : TSDescriptorVector existing(all.begin(), all.end()); : shared_ptr<TSDescriptor> extra_ts; : const auto s = policy.PlaceExtraTabletReplica(existing, label, &extra_ts); : ASSERT_TRUE(s.IsNotFound()) << s.ToString(); : ASSERT_FALSE(extra_ts); : ASSERT_STR_CONTAINS(s.ToString(), : "could not select location for extra replica"); : } : } : } : : TEST_F(PlacementPolicyTest, PlaceTabletReplicasNoLoc) { : // 'No location case': expecting backward-compatible behavior with the : // legacy (i.e. non-location-aware) logic. : c > For the purpose of readability and easier comprehension, could you put the Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@341 PS18, Line 341: ctor r > nit: drop Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@365 PS18, Line 365: const vector<LocationInfo> cluster_info = { : { "A", { { "A_ts0", 2 }, { "A_ts1", 1 }, { "A_ts2", 3 }, } }, : { "B", { { "B_ts0", 1 }, { "B_ts1", 2 }, { "B_ts2", 3 }, } }, : { "C", { { "C_ts0", 10 }, } }, : }; : ASSERT_OK(Prepare(cluster_info)); : : const auto& all = descriptors(); : PlacementPolicy policy(all, rng()); : : // Ask for number of replicas equal to the number of available locations. : { : TSDescriptorVector result; : ASSERT_OK(policy.PlaceTabletReplicas(3, none, &result)); : ASSERT_EQ(3, result.size()); : TSDescriptorsMap m; : ASSERT_OK(TSDescriptorVectorToMap(result, &m)); : ASSERT_EQ(1, m.count("A_ts0") + m.count("A_ts1")); : ASSERT_EQ(1, m.count("B_ts0") + m.count("B_ts1")); : ASSERT_EQ(0, m.count("A_ts2")); : ASSERT_EQ(0, m.count("B_ts2")); : ASSERT_EQ(1, m.count("C_ts0")); : } > Put this under for() {} scope as well? Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@389 PS18, Line 389: // Make sure no location contains the majority of replicas when there is : // enough locations to spread the replicas. : { : TSDescriptorVector result; : ASSERT_OK(policy.PlaceTabletReplicas(5, none, &result)); : ASSERT_EQ(5, result.size()); : TSDescriptorsMap m; : ASSERT_OK(TSDescriptorVectorToMap(result, &m)); : ASSERT_EQ(1, m.count("A_ts0")); : ASSERT_EQ(1, m.count("A_ts1")); : ASSERT_EQ(1, m.count("B_ts0")); : ASSERT_EQ(1, m.count("B_ts1")); : ASSERT_EQ(1, m.count("C_ts0")); : } : : // Ask for number of replicas greater than the number of tablet servers. : { : TSDescriptorVector result; : auto s = policy.PlaceTabletReplicas(8, none, &result); : ASSERT_TRUE(s.IsNotFound()) > ditto Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@963 PS18, Line 963: double stddev = calc_stddev_func(placement_stats, kMeanPerServer); : ASSERT_GE(stddev, 20.0); : } > How about: Done http://gerrit.cloudera.org:8080/#/c/13632/18/src/kudu/master/placement_policy-test.cc@971 PS18, Line 971: licy(al > nit: drop Done -- 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: 22 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: Mon, 15 Jul 2019 07:23:12 +0000 Gerrit-HasComments: Yes
