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 15: (12 comments) > (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? Yes, I will support 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 all tablets created at table creation time. > "Set the dimension label for all tablets created at table creation time". Done http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/client/client.h@879 PS12, Line 879: /// @note By default or masters doesn't set '--master_place_tablet_replicas_based_on_dimension', > This needs a more thorough explanation. Reading this, I have no idea what " Done http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/client/client.h@1217 PS12, Line 1217: KuduPartialRow* lower_bound, > 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. Thanks for pointing this out, I think we need to add a new interface to fix this issue. 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 Done http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@3715 PS12, Line 3715: optional<string > Elsewhere you use optional without the boost:: prefix. Pick one approach an Done 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 && : tablet_->metadata().state().pb.has_dimension_label()) { > Combine into one if statement. Done http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@4668 PS12, Line 4668: one. > enabled Done http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/catalog_manager.cc@4671 PS12, Line 4671: tablet->metadata().state().pb.has_dimension_label()) { : dimension = tablet->metadata().state().pb.dimension_label( > Combine Done 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: // Indicates that the tablet server needs to report the number of tablets > Doc what this means. Also, why does it exist at all? What value does select Done http://gerrit.cloudera.org:8080/#/c/13632/12/src/kudu/master/master.proto@472 PS12, Line 472: > "The dimension label for tablets that were created during table creation." Done 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(const boost::optional<std::string>& dimension = boost::none) const { > Odd that the optional isn't passed by cref, given that we're not std::move' Done 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: TabletNumByDimensionMap GetNumLiveTabletsByDimension() const; > Could we just return the map? 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: 15 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: Wed, 10 Jul 2019 10:32:36 +0000 Gerrit-HasComments: Yes
