Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15841 )
Change subject: KUDU-3090: Native owner metadata in Kudu ...................................................................... Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/client/client.h@1308 PS10, Line 1308: new_owner What is the semantics of supplying an empty string here? Is it a valid call? Overall, do you mind adding a few test scenarios for the AlterTable involving the newly introduced owner functionality? http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc@1438 PS10, Line 1438: Status ValidateOwner(const string& name) { > Mind adding a quick test to validate the behavior when the length is too lo +1 http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc@1879 PS10, Line 1879: if (req.has_owner()) { nit: does it make sense to add DCHECK(!req.owner().empty()) ? http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc@2522 PS10, Line 2522: const optional<const string&>& new_table_name, : const optional<const string&>& new_table_owner, Is it possible http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc@2529 PS10, Line 2529: is_initialized We are using boost 1.61, but in newer versions this method is deprecated: https://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/reference/header__boost_optional_optional_hpp_/detailed_semantics___optional_values.html#reference_optional_is_initialized Would it work if using operator bool(): https://www.boost.org/doc/libs/1_64_0/libs/optional/doc/html/boost_optional/reference/header__boost_optional_optional_hpp_/detailed_semantics___optional_values.html#reference_optional_operator_bool ? http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/catalog_manager.cc@2674 PS10, Line 2674: RETURN_NOT_OK(SetupError( nit: add DCHECK(!req.new_table_owner().empty()) ? http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc File src/kudu/master/hms_notification_log_listener.cc: http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc@385 PS10, Line 385: before_table.owner != after_table.owner Is it possible to dis-own a table (i.e. change a valid owner to boost::none)? http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc@389 PS10, Line 389: is_initialized This method is deprecated in newer versions of the boost library. http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/master/hms_notification_log_listener.cc@395 PS10, Line 395: *table_id, before_table_name style nit: put these two arguments at different lines as well? http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/15841/10/src/kudu/tools/kudu-tool-test.cc@3809 PS10, Line 3809: optional<string>& I saw in other places the type of parameter is optional<string&> and alike. Is it possible to converge on the type, so boost::optional<string>& is used in other methods related to the owner which use the boost::optional wrapper? -- To view, visit http://gerrit.cloudera.org:8080/15841 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67f5bfdf56d409960365fd5803913a2d3800831d Gerrit-Change-Number: 15841 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 07 Jul 2020 05:30:46 +0000 Gerrit-HasComments: Yes
