Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16160 )
Change subject: Add refreshing Ranger policies ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/16160/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16160/1//COMMIT_MSG@24 PS1, Line 24: This means that even if Ranger itself is down, we : can still get an OK response when calling this method and there's no way : to guarantee clearing the cache. Can we also clear the local cached state of the Ranger client when requesting a refresh so we fail closed in the case Ranger is down? 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@128 PS1, Line 128: plugin.refreshPoliciesAndTags(); It looks like this method catches any exceptions and only logs: } catch (InterruptedException var6) { LOG.error("Failed to update policy-engine, continuing to use old policy-engine and/or tags", var6); } Can we file a Ranger Jira to get that fixed so we can appropriately handle the failure and propagate it back at some point in the future? Then mark this with a TODO. http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/master/master.proto@942 PS1, Line 942: message ResetAuthzCacheRequestPB { Similar to the tools comments, should this be Refresh? http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/ranger/mini_ranger.h File src/kudu/ranger/mini_ranger.h: http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/ranger/mini_ranger.h@191 PS1, Line 191: 200ms so that tests don't have to wait too long until freshly : // created policies can be used. Adjust this comment. Is 30s matching the ranger default? http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/ranger/ranger_client.h@109 PS1, Line 109: // Refreshes policies in the Ranger subprocess. Can you describe a bit more about what refreshing means? Does it fetch all polices aggressively? Does it clear out local state and fetch them lazily? http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/ranger/ranger_client.cc@342 PS1, Line 342: "-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5005", What is this change for? 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@319 PS1, Line 319: Reset Is "refresh" a more accurate description? If I understand correctly this will get new/updated policies from ranger if ranger is healthy. I guess this applies to the action name and method names too. http://gerrit.cloudera.org:8080/#/c/16160/1/src/kudu/tools/tool_action_master.cc@329 PS1, Line 329: a Kudu Master The actions operate on all the kudu masters right? -- 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: 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 14:30:41 +0000 Gerrit-HasComments: Yes