----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34925/#review110435 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 33) <https://reviews.apache.org/r/34925/#comment170301> instead of "sentry shell" consider making this the "sentry admin tool". Add class comment on usage info. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 54) <https://reviews.apache.org/r/34925/#comment170302> Do we need test-only flags in the code? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 60) <https://reviews.apache.org/r/34925/#comment170298> nit: trailing whitepace here and elsewhere. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 117) <https://reviews.apache.org/r/34925/#comment170297> I think the privilege strings are a bit hard to work with. It's okay to keep this an option, but it would be clearer to split this up to have something like: --action=SELECT --scope=table --name=foo.bar sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java (line 43) <https://reviews.apache.org/r/34925/#comment170300> Add comment sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java (line 50) <https://reviews.apache.org/r/34925/#comment170303> consider breaking this method up info a few smaller pieces. Consider adding a bit more abstraction on top of the calls too. Example: executeCmd(Command cmd) { cmd.execute(); } Then have different classes for each command type: CreateRoleCommand cmd { String roleName; public CreateRoleCommand(String roleName); override execute() { thriftClient.createRole("roleName"); } } - Lenni Kuff On June 2, 2015, 5:09 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34925/ > ----------------------------------------------------------- > > (Updated June 2, 2015, 5:09 a.m.) > > > Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar. > > > Repository: sentry > > > Description > ------- > > Create a simpler shell for sentry that handles command line arguments, eg > sentryShell --grant_role_privilege --role analyst --privilege > server=server1->database=db2->table=tab1->action=select > > > Diffs > ----- > > bin/sentryShell PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/34925/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
