Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 11: (13 comments) http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@326 PS11, Line 326: // Functor that performs certain operation (e.g Create, Rename) on a table given the : // table name (and the new name to renamed to) Nit: to emphasize that the second argument is only used for Rename, perhaps wrap it in a boost::optional? Then you could document it as "Functor that performs a certain operation (e.g. Create, Rename) on a table given its name and, optionally, its desired new name. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/integration-tests/master_sentry-itest.cc@331 PS11, Line 331: // on a table given the database the table belong to and the name of the table. Nit: belongs http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.h@592 PS11, Line 592: // The RPC context is provided for retrieving the user information, : // but this function does not itself respond to the RPC. There are a bunch of these comments still sprinkled around. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1781 PS11, Line 1781: string table_name = table_identifier.has_table_name() ? : NormalizeTableName(table_identifier.table_name()) : ""; Might be clearer if table_name were wrapped in boost::optional, to reflect its optionality. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1797 PS11, Line 1797: // Indicating if the table name and table ID refer to different tables : // respectively. Nit: "Set to true if the client-provided table name and ID refer to different tables." http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1799 PS11, Line 1799: bool mismatch_table = false; Nit: mismatched_table http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1821 PS11, Line 1821: operation operate http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1824 PS11, Line 1824: RETURN_NOT_OK(authorize(table_name)); If the client provided a table ID but no table was found, we'll end up calling authorize(""). That seems like a waste of time, no? http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1830 PS11, Line 1830: : // Validate if the operation on the table is authorized. Reset the table : // name in case the client provided a table identifier with an ID but : // without a name. We're also resetting the table name in case the table got renamed just as we locked it, right? If so, may want to add that to the comment. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1840 PS11, Line 1840: if (mismatch_table) { Would be great to add test coverage for this case. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1843 PS11, Line 1843: refer refers http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@1850 PS11, Line 1850: NormalizeTableName(lock.data().name() Can reuse table_name here. http://gerrit.cloudera.org:8080/#/c/11797/11/src/kudu/master/catalog_manager.cc@2689 PS11, Line 2689: optional<const string&> user = rpc ? : make_optional<const string&>(rpc->remote_user().username()) : none; Should plumb this up into master_service.cc. -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 15 Mar 2019 07:54:25 +0000 Gerrit-HasComments: Yes
