Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 10: (12 comments) The failure test is probably due to the recent change I had in https://gerrit.cloudera.org/c/15074/10/java/kudu-subprocess-echo/build.gradle#24. I will look into it. http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/common/table_util.h File src/kudu/common/table_util.h: http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/common/table_util.h@46 PS10, Line 46: Status nit: can you comment on when the returned Status will not be ok? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.h File src/kudu/ranger/ranger_action.h: http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.h@39 PS10, Line 39: ActionPB ActionToActionPB(const Action& action); nit: Can you add comments for the methods in this class. http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.cc File src/kudu/ranger/ranger_action.cc: http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.cc@46 PS10, Line 46: return ActionPB::METADATA; LOG(FATAL) for unknown action? Similar to https://github.com/apache/kudu/blob/master/src/kudu/sentry/sentry_action.cc#L50 http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.cc@70 PS10, Line 70: } Same here. http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.h@45 PS10, Line 45: and client nit: remove? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@54 PS10, Line 54: return server_->Init(); Add a logging? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@93 PS10, Line 93: if (resp_list.responses() DCHECK the response size? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@97 PS10, Line 97: Substitute("User %s is not authorized to " : "perform %s on %s", Should we add a debug logging if the user is not authorized? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@134 PS10, Line 134: req_list Should we check the req_list size is the same as resp_list? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@162 PS10, Line 162: non_ranger_tables nit: maybe name it to invalid_ranger_tables? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@178 PS10, Line 178: non_ranger_tables.emplace_back(table); Add a log here for invalid ranger tables? http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_client.cc@193 PS10, Line 193: for (auto i = 0; i < non_ranger_tables.size(); ++i) { : table_names->emplace(non_ranger_tables[i]); : } why we place the invalid ranger table back to table_names? Shouldn't we deny invalid tables by default? -- To view, visit http://gerrit.cloudera.org:8080/15206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4 Gerrit-Change-Number: 15206 Gerrit-PatchSet: 10 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 25 Feb 2020 23:31:43 +0000 Gerrit-HasComments: Yes
