Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13632 )
Change subject: KUDU-2823 Place tablet replicas based on dimension ...................................................................... Patch Set 14: (12 comments) Just passing through; didn't look at the placement policy or testing changes. Do you intend to roll this out for the Java client API in a follow-on patch? http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/client/client.h@877 PS12, Line 877: /// Set the dimension label for the tablet that was created when the table creating. "Set the dimension label for all tablets created at table creation time". http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/client/client.h@879 PS12, Line 879: /// Used by the master to determine load when creating new tablet replicas based on dimension. This needs a more thorough explanation. Reading this, I have no idea what "load" means, nor what "creating new replicas based on dimension" means. http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/client/client.h@1217 PS12, Line 1217: const std::string& dimension_label = ""); This is a backwards incompatible change. See https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B for details on what sort of changes are permitted to retain backwards compatibility. In this case, you'll need to add a new variant of AddRangePartition() with the additional argument. You can't add an overload, because the existing AddRangePartition did not have any overloads, and adding one now would make existing usages of &AddRangePartition ambiguous. http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@3713 PS12, Line 3713: enable enabled http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@3715 PS12, Line 3715: boost::optional Elsewhere you use optional without the boost:: prefix. Pick one approach and use it consistently. http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@3716 PS12, Line 3716: if (FLAGS_master_place_tablet_replicas_based_on_dimension) { : if (tablet_->metadata().state().pb.has_dimension_label()) { Combine into one if statement. http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@4668 PS12, Line 4668: enable enabled http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@4671 PS12, Line 4671: if (FLAGS_master_place_tablet_replicas_based_on_dimension) { : if (tablet->metadata().state().pb.has_dimension_label()) { Combine http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/master.proto@365 PS12, Line 365: optional bool needs_num_live_tablets_by_dimension = 10 [ default = false ]; Doc what this means. Also, why does it exist at all? What value does selectively enabling/disabling this portion of the heartbeat add? http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/master.proto@472 PS12, Line 472: // The dimension label for tablet that was created when the table creating. "The dimension label for tablets that were created during table creation." http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/ts_descriptor.h File src/kudu/master/ts_descriptor.h: http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/ts_descriptor.h@138 PS12, Line 138: int num_live_replicas(boost::optional<std::string> dimension = boost::none) const { Odd that the optional isn't passed by cref, given that we're not std::move'ing it into a class member. http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/tserver/ts_tablet_manager.h@205 PS12, Line 205: void GetNumLiveTabletsByDimension(TabletNumByDimensionMap* num_live_tablets_by_dimension) const; Could we just return the map? -- 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: 14 Gerrit-Owner: Yao Xu <ocla...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.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 <ocla...@gmail.com> Gerrit-Comment-Date: Tue, 09 Jul 2019 03:49:28 +0000 Gerrit-HasComments: Yes
