Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15841 )
Change subject: KUDU-3090: Native owner metadata in Kudu ...................................................................... Patch Set 10: (9 comments) > Patch Set 10: > > I think there are 2 valid options. > 1. leave the check and adjust the tests. > 2. remove the check and ensure a null/empty value results in defaulting to > the current user. > > I don't feel too strongly about either approach. This is similar to the > Precondition comments on the Java client patch. So we should be consistent > whichever way we choose. The second one wouldn't work as the point of that test was to have an empty owner (i.e. upgrading from an earlier version without owner support). I ended up adding a hidden flag to allow empty owner for testing. 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) { > +1 Done 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()) ? don't think so, we check for that in the validator. Also, I need to be able to disable that validation for tests. 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 I think you missed the end of your question. 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: h Done 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()) ? same as above, we check for it in the validator. 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 I don't think so, if this is set to none, the owner is unchanged. 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. Done 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? Done 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. Done -- 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 11:06:05 +0000 Gerrit-HasComments: Yes
