Re: Review Request 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-19 Thread Vamsee Yarlagadda 


> On Dec. 19, 2016, 8:15 p.m., Hao Hao wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 1720
> > 
> >
> > It looks like fromNULLCol is handling null string representation 
> > "__NULL__" as well. If we remove processing of such string, we need to 
> > handle it when processing TSentryPriviege? Fail to do so, may bring API 
> > backward incompatibility.

Good catch. I will see what i can do here.


- Vamsee


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review159644
---


On Dec. 16, 2016, 9:17 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 16, 2016, 9:17 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  868e67720196f443cd281ec4e80ad552bf86e569 
> 
> Diff: https://reviews.apache.org/r/54525/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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-19 Thread Hao Hao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review159644
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 1720)


It looks like fromNULLCol is handling null string representation "__NULL__" 
as well. If we remove processing of such string, we need to handle it when 
processing TSentryPriviege? Fail to do so, may bring API backward 
incompatibility.


- Hao Hao


On Dec. 16, 2016, 9:17 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 16, 2016, 9:17 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  868e67720196f443cd281ec4e80ad552bf86e569 
> 
> Diff: https://reviews.apache.org/r/54525/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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-16 Thread kalyan kumar kalvagadda 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review159504
---




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 (line 1644)


I'm just confused here. What are we acheiving here. All we are trying to do 
is retain the old behaviour which could be wrong.

If we want to retain the old behaviour we rather not do any code changes.


- kalyan kumar kalvagadda


On Dec. 16, 2016, 9:17 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 16, 2016, 9:17 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  868e67720196f443cd281ec4e80ad552bf86e569 
> 
> Diff: https://reviews.apache.org/r/54525/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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-16 Thread Vamsee Yarlagadda 


> On Dec. 16, 2016, 8:18 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 1449
> > 
> >
> > I'd suggest a simpler approach.
> > 
> > For all these fields just do
> > 
> > Privilege.setFoo(mSentryPrivilege.getFoo())
> > 
> > So all fields which are null in mSentryPrivilege will become null in 
> > TSentryPrivilege which is exactly what we want without any extra ifs.

It looks like when we are trying to save a null string to the database (JDO) we 
seem to be passing the string "__NULL__" instead. So it is kinda of essential 
to do the check if the string is needed null by doing isNULL before setting to 
TSentryPrivilege.

Here is where we are converting to MSentryPrivilege:
https://github.com/apache/sentry/blob/55d2721c866f0820f03162d92b0a62768eea2d1b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java#L1472

And toNULLCol will replace all null strings with "__NULL__" string
https://github.com/apache/sentry/blob/55d2721c866f0820f03162d92b0a62768eea2d1b/sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java#L1720


> On Dec. 16, 2016, 8:18 p.m., Alexander Kolbasov wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java,
> >  line 1644
> > 
> >
> > I think this is in SENTRY-1540.

Reworded the comment to mean that as part of SENTRY-1541, we wanted to ensure 
to preserve the original behaviour.


- Vamsee


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review159486
---


On Dec. 16, 2016, 7:51 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 16, 2016, 7:51 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  868e67720196f443cd281ec4e80ad552bf86e569 
> 
> Diff: https://reviews.apache.org/r/54525/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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-15 Thread Vamsee Yarlagadda 


> On Dec. 13, 2016, 6:28 p.m., Alexander Kolbasov wrote:
> > 1) Do we have any other users of fromNullCol or it should be removed as 
> > well?
> > 2) This relates to SENTRY-1540 since now isMultiActionsSupported() becomes 
> > a real code and this changes the existing semantics some. Is it Ok or not?

bq. 1) Do we have any other users of fromNullCol or it should be removed as 
well?
I verified that there are no users of fromNullCol and that's why it was removed 
in the last patch.

bq. 2) This relates to SENTRY-1540 since now isMultiActionsSupported() becomes 
a real code and this changes the existing semantics some. Is it Ok or not?
As part of this change we are only ensuring thrift objects has NULL attributes 
for the fields that are not set. And this still preserves the functionality of 
isMultiActionsSupported as they are only checking for NULL variables in thrift 
objects.


- Vamsee


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review159035
---


On Dec. 12, 2016, 7:57 p.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 12, 2016, 7:57 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> Diffs
> -
> 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  e8d0e2505c5b178cae3d27e7d85caa652f630a2d 
> 
> Diff: https://reviews.apache.org/r/54525/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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-08 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review158578
---



I mean convertToTSentryPrivilege() is similar.

- Alexander Kolbasov


On Dec. 8, 2016, 8:47 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 8, 2016, 8:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> 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/54525/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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-08 Thread Alexander Kolbasov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review158576
---



There is also toSentryPrivilege() with similar problem. And once this is fixed 
we don't need fromNULLCol() anymore.

- Alexander Kolbasov


On Dec. 8, 2016, 8:47 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 8, 2016, 8:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> 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/54525/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 54525: SENTRY-1541: toSentryPrivilege() should not copy fields that are not set in the source

2016-12-08 Thread kalyan kumar kalvagadda 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54525/#review158541
---


Ship it!




Looks good

- kalyan kumar kalvagadda


On Dec. 8, 2016, 8:47 a.m., Vamsee Yarlagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54525/
> ---
> 
> (Updated Dec. 8, 2016, 8:47 a.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar 
> kalvagadda.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Only sets the fields in TSentryPrivilege that are set in TSentryAuthorizable
> 
> 
> 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/54525/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
> 
>