You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Colin Ma <ju...@intel.com> on 2015/06/02 07:10:00 UTC
Review Request 34925: SENTRY-749: Create simple shell for sentry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/
-----------------------------------------------------------
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
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
> On June 17, 2015, 12:15 a.m., Prasad Mujumdar wrote:
> > Mostly look fine. A couple of questions -
> > - Does the sentry config file need to be in classpath ? can it be specified via another argument ?
> > - I guess it will need some work for secure connection. The DB client needs the actions to be wrapped in security context. In case of Hive or other service, they already have secure UGI object created. In case of the shell we'll need to handle it.
> > - We should definately log a followup ticket for generic privileges.
> >
> > Few minor comments/suggestions below
Thanks for the comments,
1. The sentry config file should be spcified via the argument -conf <filepath> or --sentry_conf <filepath>
2. For the security feature, we can trace this feature by another ticket.
3. For generic model, we also need fire a new ticket.
> On June 17, 2015, 12:15 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, line 238
> > <https://reviews.apache.org/r/34925/diff/1/?file=976351#file976351line238>
> >
> > Isn't this already verified on the server side. In that case, we don't have duplicate the validation logic in shell again.
For this privilege hierarchy, there is no validation logic on the server side. In the previous, maybe the privilege was generated from hive, so such validation is not neccessary, in this feature, the privilege is generated from the shell, to avoid the mistype, I think this validation should be kept.
> On June 17, 2015, 12:15 a.m., Prasad Mujumdar wrote:
> > bin/sentryShell, line 1
> > <https://reviews.apache.org/r/34925/diff/1/?file=976349#file976349line1>
> >
> > It would be good to have an option to invoke this from sentry script itself.
> > Perhaps you can log a followup ticket to address that in future.
Do you mean the shell should be called like the following example:
sentry --simpleshell -t hive -gpr -r testrole -p server=server1->db=db1->action=select -conf /path/to/configfile ?
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review88171
-----------------------------------------------------------
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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Prasad Mujumdar <pr...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review88171
-----------------------------------------------------------
Mostly look fine. A couple of questions -
- Does the sentry config file need to be in classpath ? can it be specified via another argument ?
- I guess it will need some work for secure connection. The DB client needs the actions to be wrapped in security context. In case of Hive or other service, they already have secure UGI object created. In case of the shell we'll need to handle it.
- We should definately log a followup ticket for generic privileges.
Few minor comments/suggestions below
bin/sentryShell (line 1)
<https://reviews.apache.org/r/34925/#comment140578>
It would be good to have an option to invoke this from sentry script itself.
Perhaps you can log a followup ticket to address that in future.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java (line 238)
<https://reviews.apache.org/r/34925/#comment140579>
Isn't this already verified on the server side. In that case, we don't have duplicate the validation logic in shell again.
- Prasad Mujumdar
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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
> On Dec. 8, 2015, 8:41 a.m., Hao Hao wrote:
> > bin/sentryShell, line 1
> > <https://reviews.apache.org/r/34925/diff/1/?file=976349#file976349line1>
> >
> > Could you please also fix the unittest?
The test failed is not caused by this patch, I created SENTRY-981 for the test failed.
> On Dec. 8, 2015, 8:41 a.m., Hao Hao wrote:
> > bin/sentryShell, line 43
> > <https://reviews.apache.org/r/34925/diff/1/?file=976349#file976349line43>
> >
> > Should we also make _CMD_JAR user definable?
Good point, update the patch and SENTRY_SHELL_JAR can be used as user definable.
> On Dec. 8, 2015, 8:41 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 62
> > <https://reviews.apache.org/r/34925/diff/1/?file=976350#file976350line62>
> >
> > change the "create role" comment to "sentry config file path".
thanks for catch it.
> On Dec. 8, 2015, 8:41 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java, line 420
> > <https://reviews.apache.org/r/34925/diff/1/?file=976352#file976352line420>
> >
> > Should we assert the exception type?
Good point, I'll use the specific exception(eg, SentryUserException) instead of Exception.
> On Dec. 8, 2015, 8:41 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 199
> > <https://reviews.apache.org/r/34925/diff/1/?file=976350#file976350line199>
> >
> > Will the required option be speficed as required in the printed usage?
If required option is missed, there will be a ParseException thrown to warn the user.
> On Dec. 8, 2015, 8:41 a.m., Hao Hao wrote:
> > bin/sentryShell, line 58
> > <https://reviews.apache.org/r/34925/diff/1/?file=976349#file976349line58>
> >
> > Remove all the extra spaces.
done
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review109296
-----------------------------------------------------------
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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review109296
-----------------------------------------------------------
bin/sentryShell (line 1)
<https://reviews.apache.org/r/34925/#comment168762>
Could you please also fix the unittest?
bin/sentryShell (line 43)
<https://reviews.apache.org/r/34925/#comment168758>
Should we also make _CMD_JAR user definable?
bin/sentryShell (line 58)
<https://reviews.apache.org/r/34925/#comment168759>
Remove all the extra spaces.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 62)
<https://reviews.apache.org/r/34925/#comment168764>
change the "create role" comment to "sentry config file path".
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 199)
<https://reviews.apache.org/r/34925/#comment168765>
Will the required option be speficed as required in the printed usage?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java (line 420)
<https://reviews.apache.org/r/34925/#comment168761>
Should we assert the exception type?
- Hao Hao
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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, line 19
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165630#file1165630line19>
> >
> > Everything below here looks hive specific. Put in sentry.provider.db.tools.hive package?
>
> Colin Ma wrote:
> Yes, it's for hive only and the class name is suffix with Hive. But for the package org.apache.sentry.provider.db.tools.command, I move it to org.apache.sentry.provider.db.tools.command.**hive** as your suggestion, thanks.
looks good to me, thanks.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 196
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165629#file1165629line196>
> >
> > Missing parameter: + paramName
>
> Colin Ma wrote:
> Thanks, this is updated as Missing required option: as the same as the error message from GnuParser.parse.
looks good!
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > bin/sentryShell, line 53
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165628#file1165628line53>
> >
> > why are you using HADOOP_CLASSPATH? Where is that used?
>
> Colin Ma wrote:
> The shell is started by the command like:
> exec $HADOOP jar ${SENTRY_HOME}/lib/${_CMD_JAR} org.apache.sentry.SentryShellHive ${args[@]}
> HADOOP_CLASSPATH is for this command.
Why execute it through the hadoop command rather than having it be a stand alone shell?
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 67
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165629#file1165629line67>
> >
> > At least with the file-based backend, a role was made up of groups. So adding a role to group is backwards. Am I missing something?
>
> Colin Ma wrote:
> The target for this tools is only for database-based backend. For the file-based, I think modify the configuration file is more convenient and it isn't supported by this tool.
I think you misunderstand. I'm not saying this needs to work for the file-based backend. I'm saying that it should be "add group to role" not "add role to group".
Put another way:
A group is a set of users.
A role is a set of groups.
You don't say "add group to user" you say "add user to group"
So by analogy, you don't say "add role to group" you say "add group to role"
- Gregory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review111569
-----------------------------------------------------------
On Dec. 23, 2015, 8:42 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34925/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2015, 8:42 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/hive/Command.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > bin/sentryShell, line 53
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165628#file1165628line53>
> >
> > why are you using HADOOP_CLASSPATH? Where is that used?
The shell is started by the command like:
exec $HADOOP jar ${SENTRY_HOME}/lib/${_CMD_JAR} org.apache.sentry.SentryShellHive ${args[@]}
HADOOP_CLASSPATH is for this command.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 67
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165629#file1165629line67>
> >
> > At least with the file-based backend, a role was made up of groups. So adding a role to group is backwards. Am I missing something?
The target for this tools is only for database-based backend. For the file-based, I think modify the configuration file is more convenient and it isn't supported by this tool.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 196
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165629#file1165629line196>
> >
> > Missing parameter: + paramName
Thanks, this is updated as Missing required option: as the same as the error message from GnuParser.parse.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, line 19
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165630#file1165630line19>
> >
> > Everything below here looks hive specific. Put in sentry.provider.db.tools.hive package?
Yes, it's for hive only and the class name is suffix with Hive. But for the package org.apache.sentry.provider.db.tools.command, I move it to org.apache.sentry.provider.db.tools.command.**hive** as your suggestion, thanks.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review111569
-----------------------------------------------------------
On Dec. 23, 2015, 8:42 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34925/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2015, 8:42 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/hive/Command.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 67
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165629#file1165629line67>
> >
> > At least with the file-based backend, a role was made up of groups. So adding a role to group is backwards. Am I missing something?
>
> Colin Ma wrote:
> The target for this tools is only for database-based backend. For the file-based, I think modify the configuration file is more convenient and it isn't supported by this tool.
>
> Gregory Chanan wrote:
> I think you misunderstand. I'm not saying this needs to work for the file-based backend. I'm saying that it should be "add group to role" not "add role to group".
> Put another way:
> A group is a set of users.
> A role is a set of groups.
>
> You don't say "add group to user" you say "add user to group"
> So by analogy, you don't say "add role to group" you say "add group to role"
I got it now, thanks for point that, and I updated all related comments as your suggestion.
> On Dec. 22, 2015, 2:26 a.m., Gregory Chanan wrote:
> > bin/sentryShell, line 53
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165628#file1165628line53>
> >
> > why are you using HADOOP_CLASSPATH? Where is that used?
>
> Colin Ma wrote:
> The shell is started by the command like:
> exec $HADOOP jar ${SENTRY_HOME}/lib/${_CMD_JAR} org.apache.sentry.SentryShellHive ${args[@]}
> HADOOP_CLASSPATH is for this command.
>
> Gregory Chanan wrote:
> Why execute it through the hadoop command rather than having it be a stand alone shell?
This script is implemented as the script **bin/sentry**. When run the script, there will be some Hadoop dependency needed, and use hadoop command can process these dependency well.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review111569
-----------------------------------------------------------
On Dec. 24, 2015, 5:52 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34925/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2015, 5:52 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/hive/Command.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review111569
-----------------------------------------------------------
bin/sentryShell (line 53)
<https://reviews.apache.org/r/34925/#comment171768>
why are you using HADOOP_CLASSPATH? Where is that used?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 67)
<https://reviews.apache.org/r/34925/#comment171770>
At least with the file-based backend, a role was made up of groups. So adding a role to group is backwards. Am I missing something?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 68)
<https://reviews.apache.org/r/34925/#comment171771>
same here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 106)
<https://reviews.apache.org/r/34925/#comment171769>
simpleShellOptGroup is misspelled.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java (line 196)
<https://reviews.apache.org/r/34925/#comment171776>
Missing parameter: + paramName
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java (line 19)
<https://reviews.apache.org/r/34925/#comment171777>
Everything below here looks hive specific. Put in sentry.provider.db.tools.hive package?
- Gregory Chanan
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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Gregory Chanan <gc...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review112110
-----------------------------------------------------------
Ship it!
Looks fine to me for the non-hive stuff. For the hive stuff, I'll defer to someone who knows that area better.
- Gregory Chanan
On Dec. 24, 2015, 5:52 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34925/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2015, 5:52 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/hive/Command.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review112478
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/Command.java (line 26)
<https://reviews.apache.org/r/34925/#comment172927>
public can be remove since it is a method of public interface.
- Hao Hao
On Dec. 24, 2015, 5:52 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34925/
> -----------------------------------------------------------
>
> (Updated Dec. 24, 2015, 5:52 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/hive/Command.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/
-----------------------------------------------------------
(Updated Dec. 24, 2015, 5:52 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 (updated)
-----
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/hive/Command.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/
-----------------------------------------------------------
(Updated Dec. 23, 2015, 8:42 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 (updated)
-----
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/hive/Command.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
> On Dec. 21, 2015, 11:20 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, line 61
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165630#file1165630line61>
> >
> > should we add some log for the command == null case?
Thanks for point that, this will be checked by GnuParser.parse, and I also add the test case for missing command option.
> On Dec. 21, 2015, 11:20 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellHive.java, line 62
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165630#file1165630line62>
> >
> > should we handle the case when requestorName.isEmpty() here?
Thanks, the check is added.
> On Dec. 21, 2015, 11:20 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java, line 404
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165641#file1165641line404>
> >
> > 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.
Thaks for point that, the method name is changed as testNegativeCaseWithInvalidArgument and testNegativeCaseWithoutRequiredArgument.
> On Dec. 21, 2015, 11:20 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentryShellHive.java, line 489
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165641#file1165641line489>
> >
> > 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)
Good point, I update the tests to verify the error message.
> On Dec. 21, 2015, 11:20 p.m., Li Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java, line 145
> > <https://reviews.apache.org/r/34925/diff/2/?file=1165629#file1165629line145>
> >
> > how about adding a --help option to print out simpleShellOptions
Thanks for point that, I'll add this option.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34925/#review111540
-----------------------------------------------------------
On Dec. 23, 2015, 8:42 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34925/
> -----------------------------------------------------------
>
> (Updated Dec. 23, 2015, 8:42 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/hive/Command.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CommandUtil.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/CreateRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/DropRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantPrivilegeToRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/GrantRoleToGroupsCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListPrivilegesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/ListRolesCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/RevokePrivilegeFromRoleCmd.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/command/hive/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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Li Li <li...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Colin Ma <ju...@intel.com>.
> 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
>
>
Re: Review Request 34925: SENTRY-749: Create simple shell for sentry
Posted by Lenni Kuff <ls...@cloudera.com>.
-----------------------------------------------------------
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
>
>