Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16160 )
Change subject: Add refreshing Ranger policies ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/16160/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java: http://gerrit.cloudera.org:8080/#/c/16160/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@125 PS1, Line 125: if (LOG.isDebugEnabled()) { nit: I don't think you need this if guard (and below). It's generally only useful if computing the log message is expensive. http://gerrit.cloudera.org:8080/#/c/16160/1/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ranger/authorization/RangerKuduAuthorizer.java@128 PS1, Line 128: plugin.refreshPoliciesAndTags(); > It only catches InterruptedExceptions though, other exceptions are not even It would be nice if it returned a boolean to indicate success at a minimum, they could add a new method to support that in a compatible way. I was thinking we could use the policy version, but that won't work in the case there are no changes. plugin.getCurrentRangerAuthContext().getPolicyEngine().getPolicyVersion(); I am okay to leave this for now. Would you mind adding a comment here about the best-effort refresh here as well? http://gerrit.cloudera.org:8080/#/c/16160/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/16160/2/src/kudu/master/master_service.cc@728 PS2, Line 728: reset nit: refresh http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/tools/tool_action_master.cc@317 PS1, Line 317: unique_ptr<Action> action_reset = Can you add a test for the tool? -- To view, visit http://gerrit.cloudera.org:8080/16160 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie90b1c23b92060e081ab514fbae417b4f31c2b66 Gerrit-Change-Number: 16160 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 09 Jul 2020 15:41:16 +0000 Gerrit-HasComments: Yes
