You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Vamsee Yarlagadda <va...@cloudera.com> on 2016/12/08 09:02:40 UTC
Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
Repository: sentry
Description
-------
Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
Change based on: https://reviews.apache.org/r/54525/
Should we remove the method as the whole and do the DB null check directly at the if clauses?
Diffs
-----
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
Diff: https://reviews.apache.org/r/54526/diff/
Testing
-------
```bash
vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
...
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
Results :
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.421s
[INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
[INFO] Final Memory: 67M/687M
[INFO] ------------------------------------------------------------------------
```
Thanks,
Vamsee Yarlagadda
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1624
> > <https://reviews.apache.org/r/54526/diff/2/?file=1596329#file1596329line1624>
> >
> > Do you know why isMultiActionsSupported is needed? The existing comment is not quite clear to me.
Updated the description of the review to address your question.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review160558
-----------------------------------------------------------
On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2017, 8:15 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> 1.
> isMultiActionsSupported used to return true for all invocations as tPrivilege.getDbName() is never set to NULL.
> So we should change it instead to do a proper check for null - SentryStore.isNull() instead.
>
> 2. Currently this method is only dealing database name string but it should also check for table name strings as mentioned in the comments of the above method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level privileges". So fixes the method to handle tables as well.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1624
> > <https://reviews.apache.org/r/54526/diff/2/?file=1596329#file1596329line1624>
> >
> > Do you know why isMultiActionsSupported is needed? The existing comment is not quite clear to me.
>
> Vamsee Yarlagadda wrote:
> Updated the description of the review to address your question.
This method is used to check which entities support multiple actions (QUERY, UPDATE, ALL). As per the comment, it looks like Database and Table are the ones that support all three. Do you think even Server, Column support all actions? If so, we can get rid of the entire method.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review160558
-----------------------------------------------------------
On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2017, 8:15 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> 1.
> isMultiActionsSupported used to return true for all invocations as tPrivilege.getDbName() is never set to NULL.
> So we should change it instead to do a proper check for null - SentryStore.isNull() instead.
>
> 2. Currently this method is only dealing database name string but it should also check for table name strings as mentioned in the comments of the above method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level privileges". So fixes the method to handle tables as well.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Hao Hao <ha...@cloudera.com>.
> On Jan. 5, 2017, 3:49 a.m., Hao Hao wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1624
> > <https://reviews.apache.org/r/54526/diff/2/?file=1596329#file1596329line1624>
> >
> > Do you know why isMultiActionsSupported is needed? The existing comment is not quite clear to me.
>
> Vamsee Yarlagadda wrote:
> Updated the description of the review to address your question.
>
> Vamsee Yarlagadda wrote:
> This method is used to check which entities support multiple actions (QUERY, UPDATE, ALL). As per the comment, it looks like Database and Table are the ones that support all three. Do you think even Server, Column support all actions? If so, we can get rid of the entire method.
I am also not quite sure here. As this code exist quite a while should we change its behavior before we are complete clear?
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review160558
-----------------------------------------------------------
On Jan. 9, 2017, 8:15 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2017, 8:15 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> 1.
> isMultiActionsSupported used to return true for all invocations as tPrivilege.getDbName() is never set to NULL.
> So we should change it instead to do a proper check for null - SentryStore.isNull() instead.
>
> 2. Currently this method is only dealing database name string but it should also check for table name strings as mentioned in the comments of the above method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level privileges". So fixes the method to handle tables as well.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review160558
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1624)
<https://reviews.apache.org/r/54526/#comment231704>
Do you know why isMultiActionsSupported is needed? The existing comment is not quite clear to me.
- Hao Hao
On Jan. 5, 2017, 12:53 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 12:53 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Jan. 9, 2017, 6:27 p.m., Mat Crocker wrote:
> > should your || be an && ?
> > this logic suggests its ok if the db is null, but the table name isn't
The comment for this function says: "// Currently INSERT/SELECT/ALL are supported for Table and DB level privileges"
So if the privilege has either DB or Table set, then they support multiple actions. So that's why it has to be || instead of &&.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review160920
-----------------------------------------------------------
On Jan. 5, 2017, 12:53 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 12:53 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Mat Crocker <ma...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review160920
-----------------------------------------------------------
should your || be an && ?
this logic suggests its ok if the db is null, but the table name isn't
- Mat Crocker
On Jan. 5, 2017, 12:53 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 12:53 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/
-----------------------------------------------------------
(Updated Jan. 9, 2017, 8:15 p.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
Repository: sentry
Description (updated)
-------
1.
isMultiActionsSupported used to return true for all invocations as tPrivilege.getDbName() is never set to NULL.
So we should change it instead to do a proper check for null - SentryStore.isNull() instead.
2. Currently this method is only dealing database name string but it should also check for table name strings as mentioned in the comments of the above method "/ Currently INSERT/SELECT/ALL are supported for Table and DB level privileges". So fixes the method to handle tables as well.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
Diff: https://reviews.apache.org/r/54526/diff/
Testing
-------
```bash
vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
...
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
Results :
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.421s
[INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
[INFO] Final Memory: 67M/687M
[INFO] ------------------------------------------------------------------------
```
Thanks,
Vamsee Yarlagadda
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/
-----------------------------------------------------------
(Updated Jan. 5, 2017, 12:53 a.m.)
Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
Changes
-------
Function to check for both databases and tables in the correct way.
Repository: sentry
Description
-------
Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
Change based on: https://reviews.apache.org/r/54525/
Should we remove the method as the whole and do the DB null check directly at the if clauses?
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
Diff: https://reviews.apache.org/r/54526/diff/
Testing
-------
```bash
vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
...
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
Results :
Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.421s
[INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
[INFO] Final Memory: 67M/687M
[INFO] ------------------------------------------------------------------------
```
Thanks,
Vamsee Yarlagadda
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review158542
-----------------------------------------------------------
From what I understand from SENTRY-162, is that dropPrivilege should contruct privilege objects from TSentryAuthorizable object for all the actions possible and then call dropPrivilegeForAllRoles for each of them for Table and DB level privileges.
But it is not implmented that way. We may have to fix the logic all together
We should test and see if were are properly updating/removing the privialges stroed in sentry store when the Authorizable entities are altered/droped.
- kalyan kumar kalvagadda
On Dec. 8, 2016, 9:02 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 9:02 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On Dec. 8, 2016, 9:32 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1607
> > <https://reviews.apache.org/r/54526/diff/1/?file=1579704#file1579704line1607>
> >
> > So this is just the rename - right? The point is that getDbName is never NULL. Or did you intend to check for the actual __NULL__ string here?
>
> Vamsee Yarlagadda wrote:
> With the change suggested in https://reviews.apache.org/r/54525/, getDbName could also return a NULL value. So that's why i renamed the function to check if DB is null or not or we could completely replace the function call with a conditional statement at the if clauses.
It is with the fix for SENTRY-1541 but not with the current diff. Actually, applying SENTRY-1541 changes the semantics here - suddenly this function is no longer a dummy call. So we either want to preserve the original semantics (and this means removing the function altogether) or changing to new one (in which case we should be confident that it is correct).
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review158572
-----------------------------------------------------------
On Dec. 8, 2016, 9:02 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 9:02 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Dec. 8, 2016, 9:32 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1607
> > <https://reviews.apache.org/r/54526/diff/1/?file=1579704#file1579704line1607>
> >
> > So this is just the rename - right? The point is that getDbName is never NULL. Or did you intend to check for the actual __NULL__ string here?
With the change suggested in https://reviews.apache.org/r/54525/, getDbName could also return a NULL value. So that's why i renamed the function to check if DB is null or not or we could completely replace the function call with a conditional statement at the if clauses.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review158572
-----------------------------------------------------------
On Dec. 8, 2016, 9:02 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 9:02 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Dec. 8, 2016, 9:32 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 1607
> > <https://reviews.apache.org/r/54526/diff/1/?file=1579704#file1579704line1607>
> >
> > So this is just the rename - right? The point is that getDbName is never NULL. Or did you intend to check for the actual __NULL__ string here?
>
> Vamsee Yarlagadda wrote:
> With the change suggested in https://reviews.apache.org/r/54525/, getDbName could also return a NULL value. So that's why i renamed the function to check if DB is null or not or we could completely replace the function call with a conditional statement at the if clauses.
>
> Alexander Kolbasov wrote:
> It is with the fix for SENTRY-1541 but not with the current diff. Actually, applying SENTRY-1541 changes the semantics here - suddenly this function is no longer a dummy call. So we either want to preserve the original semantics (and this means removing the function altogether) or changing to new one (in which case we should be confident that it is correct).
Reverted the function to it's original name and added support to check for tables as well.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review158572
-----------------------------------------------------------
On Jan. 5, 2017, 12:53 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 12:53 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 3f3afb7080ee330fedd48f4d400553fe14d57deb
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 54526: SENTRY-1540:
SentryStore.isMultiActionsSupported() is always true
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54526/#review158572
-----------------------------------------------------------
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1607)
<https://reviews.apache.org/r/54526/#comment229355>
So this is just the rename - right? The point is that getDbName is never NULL. Or did you intend to check for the actual __NULL__ string here?
- Alexander Kolbasov
On Dec. 8, 2016, 9:02 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54526/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 9:02 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Renames the method isMultiActionsSupported to isDBNull to reflect the right action being performed.
> Change based on: https://reviews.apache.org/r/54525/
>
> Should we remove the method as the whole and do the DB null check directly at the if clauses?
>
>
> Diffs
> -----
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java f773a4443e81c5cde3aca0056a2e33d528bf4ec9
>
> Diff: https://reviews.apache.org/r/54526/diff/
>
>
> Testing
> -------
>
> ```bash
> vamsee-MBP:sentry-service-server vamsee$ mvn clean test -Dtest=TestSentryStore -DfailIfNoTests=false
> ...
> -------------------------------------------------------
> T E S T S
> -------------------------------------------------------
> Running org.apache.sentry.provider.db.service.persistent.TestSentryStore
> 2016-12-08 00:08:59.772 java[5207:544535] Unable to load realm info from SCDynamicStore
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 18.302 sec - in org.apache.sentry.provider.db.service.persistent.TestSentryStore
>
> Results :
>
> Tests run: 41, Failures: 0, Errors: 0, Skipped: 2
>
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 44.421s
> [INFO] Finished at: Thu Dec 08 00:09:18 PST 2016
> [INFO] Final Memory: 67M/687M
> [INFO] ------------------------------------------------------------------------
> ```
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>