----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34925/#review111540 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 78) <https://reviews.apache.org/r/34925/#comment171748> make this method protected? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 145) <https://reviews.apache.org/r/34925/#comment171743> how about adding a --help option to print out simpleShellOptions sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 188) <https://reviews.apache.org/r/34925/#comment171742> since ParseException is always caught, and the exception message in checkRequiredParameter has no chance to be seen. I think we'd better print the ParseException msg. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 209) <https://reviews.apache.org/r/34925/#comment171747> make this method protected? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java (line 61) <https://reviews.apache.org/r/34925/#comment171744> should we add some log for the command == null case? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java (line 62) <https://reviews.apache.org/r/34925/#comment171745> should we handle the case when requestorName.isEmpty() here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/CommandUtil.java (line 90) <https://reviews.apache.org/r/34925/#comment171736> how about just return privilegeScope instead of privilegeScope.toString() in getPrivilegeScope() method? So that we can just use == to compare PrivilegeScope type, and do not need to use another toString() method here. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java (line 109) <https://reviews.apache.org/r/34925/#comment171749> how about remove the extra empty line? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java (line 404) <https://reviews.apache.org/r/34925/#comment171751> The name of the test method here is not intuitive. It is not easy to get what is the difference between testNegativeCase1() and testNegativeCase2(). We'd better add some comments about the type of the covered negative tests. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java (line 489) <https://reviews.apache.org/r/34925/#comment171753> For the required argument test, it is also important that the error message returned is correct. For example, here the error msg should contains("Miss parameter for " + OPTION_DESC_CONF) - Li Li On Dec. 16, 2015, 3:23 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34925/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2015, 3:23 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/main/java/org/apache/sentry/provider/db/tools/command/Command.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/CommandUtil.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/CreateRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/DropRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/GrantPrivilegeToRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/GrantRoleToGroupsCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/ListPrivilegesCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/ListRolesCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/RevokePrivilegeFromRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/RevokeRoleFromGroupsCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > 6bc9f75 > > Diff: https://reviews.apache.org/r/34925/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
