> On Dec. 15, 2015, 6:30 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, > > line 54 > > <https://reviews.apache.org/r/34925/diff/1/?file=976350#file976350line54> > > > > Do we need test-only flags in the code?
Thanks for caught that, update the logic of parse argument, and remove these flags. > On Dec. 15, 2015, 6:30 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, > > line 33 > > <https://reviews.apache.org/r/34925/diff/1/?file=976350#file976350line33> > > > > instead of "sentry shell" consider making this the "sentry admin tool". > > Add class comment on usage info. Added the comment. > On Dec. 15, 2015, 6:30 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, > > line 43 > > <https://reviews.apache.org/r/34925/diff/1/?file=976351#file976351line43> > > > > Add comment Done > On Dec. 15, 2015, 6:30 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, > > line 117 > > <https://reviews.apache.org/r/34925/diff/1/?file=976350#file976350line117> > > > > 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 Yes, the privilege string is hard for work. Your suggestion is a good option, because this is an enhancement, I prefer to do this in another JIRA, to support both format for the privilege, is it ok? > On Dec. 15, 2015, 6:30 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, > > line 50 > > <https://reviews.apache.org/r/34925/diff/1/?file=976351#file976351line50> > > > > 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"); > > } > > } Good suggestion, done. > On Dec. 15, 2015, 6:30 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, > > line 60 > > <https://reviews.apache.org/r/34925/diff/1/?file=976350#file976350line60> > > > > nit: trailing whitepace here and elsewhere. done - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34925/#review110435 ----------------------------------------------------------- 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 > >
