Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 10: (12 comments) 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? Done 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. Done 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/bl Done http://gerrit.cloudera.org:8080/#/c/15206/10/src/kudu/ranger/ranger_action.cc@70 PS10, Line 70: } > Same here. Done 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? Done 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? Done 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? Done 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? not sure if it's worth spamming the logs with it, a future audit log might be a better place for it. What do you think? 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? Done 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? Done 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? if we choose the approach I explained below I don't think it's necessary, maybe vlog? 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 den as they can't be managed by Ranger anyway I thought it would be better to retain existing functionality which is allow everything for authenticated users. -- 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: Wed, 26 Feb 2020 11:51:07 +0000 Gerrit-HasComments: Yes
