You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/04/07 17:24:51 UTC

Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

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

Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.

Bugs: SENTRY-1629
https://issues.apache.org/jira/browse/SENTRY-1629

Repository: sentry

Description
-------

Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.

Submitted code changes address this issue. Here is the snapshot of the changes

1. Change the JDO definition to have Path as separate entity.
2. SQL changes needed for the handle the new JDO definition.
3. Application changes to use new JDO definition.
4. Updated unit test cases to test the case where an authz object is associated with more than one path.

Diffs
-----

sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746
sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df
sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f
sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8
sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236
sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff
sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa
sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099
sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7
sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7
sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77

Diff: https://reviews.apache.org/r/58267/diff/1/

Testing
-------

Testing done:
1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
2. Added unit tests to be sure that the actual issues is addressed.

Thanks,

kalyan kumar kalvagadda

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by Hao Hao <ha...@cloudera.com>.

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


Fix it, then Ship it!





sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 59 (original), 59 (patched)
<https://reviews.apache.org/r/58267/#comment244263>

    Make it explict package.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Line 2235 (original), 2235 (patched)
<https://reviews.apache.org/r/58267/#comment244267>

    Looks like this one is redundant andb be covered by the following test? You can just remove it.



sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
Lines 2255 (patched)
<https://reviews.apache.org/r/58267/#comment244264>

    'multiple paths'?


- Hao Hao


On April 7, 2017, 5:24 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> -----------------------------------------------------------
> 
> (Updated April 7, 2017, 5:24 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
>     https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is associated with more than one path.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/1/
> 
> 
> Testing
> -------
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.


> On April 7, 2017, 9:43 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Line 31 (original), 32 (patched)
> > <https://reviews.apache.org/r/58267/diff/1/?file=1686503#file1686503line32>
> >
> >     can be final

It can not be made final. I'm not sure of the exact issue. Datanucleus has issue contructing the objects I guess.


> On April 7, 2017, 9:43 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Line 35 (original), 36 (patched)
> > <https://reviews.apache.org/r/58267/diff/1/?file=1686503#file1686503line36>
> >
> >     Instead of Set<String> it can use Iterable<STring>

What is the advantage of doing so?


> On April 7, 2017, 9:43 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Line 61 (original), 64 (patched)
> > <https://reviews.apache.org/r/58267/diff/1/?file=1686503#file1686503line64>
> >
> >     No one is calling this

This function may be used by Sentry-1587. There are other function in this class that are not used now. I did not touch them as it may be used by Sentry-1587.

There are some methods that I think may not be used. I have removed them.


> On April 7, 2017, 9:43 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/58267/diff/1/?file=1686503#file1686503line68>
> >
> >     Naming - this returns a set of paths for the object, so why is it called getPathsString?

Actual paths are not stored as Strings. We need to expose API's to get the PATH as Set<MPath> and Set<String>


> On April 7, 2017, 9:43 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
> > Lines 24 (patched)
> > <https://reviews.apache.org/r/58267/diff/1/?file=1686504#file1686504line24>
> >
> >     can it be final?

It can not be made final. I'm not sure of the exact issue. Datanucleus has issue contructing the objects I guess.


> On April 7, 2017, 9:43 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql
> > Lines 14 (patched)
> > <https://reviews.apache.org/r/58267/diff/1/?file=1686507#file1686507line23>
> >
> >     Here and in other places - why do we create table first and then declare primary key as alter table? Can we declare primary key inline?

I was just following the convention followed before. When I had constriant inline, I had comment asking to have it outside for the same reason.


- kalyan kumar


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


On April 10, 2017, 12:44 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> -----------------------------------------------------------
> 
> (Updated April 10, 2017, 12:44 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
>     https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is associated with more than one path.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/2/
> 
> 
> Testing
> -------
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by Alexander Kolbasov <ak...@gmail.com>.

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 31 (original), 32 (patched)
<https://reviews.apache.org/r/58267/#comment244277>

    can be final



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 33 (original), 34 (patched)
<https://reviews.apache.org/r/58267/#comment244289>

    I think this field should be initialized by constructor, not callers.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 35 (original), 36 (patched)
<https://reviews.apache.org/r/58267/#comment244278>

    Instead of Set<String> it can use Iterable<STring>



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 61 (original), 64 (patched)
<https://reviews.apache.org/r/58267/#comment244279>

    No one is calling this



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 68 (patched)
<https://reviews.apache.org/r/58267/#comment244281>

    Naming - this returns a set of paths for the object, so why is it called getPathsString?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 69 (patched)
<https://reviews.apache.org/r/58267/#comment244280>

    you can be more precise in defining the HashSet:
    Set<String> paths = new HashSet<>(this.paths.size());



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Lines 20 (patched)
<https://reviews.apache.org/r/58267/#comment244284>

    Please add some comment explaining why we need a wrapper over String



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Lines 24 (patched)
<https://reviews.apache.org/r/58267/#comment244282>

    can it be final?



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Line 259 (original), 258 (patched)
<https://reviews.apache.org/r/58267/#comment244283>

    This file uses 2 spaces - I think new changes use different identation



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 59 (original), 59 (patched)
<https://reviews.apache.org/r/58267/#comment244287>

    The usual convention is to avoid wildcard imports.



sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql
Lines 14 (patched)
<https://reviews.apache.org/r/58267/#comment244286>

    Here and in other places - why do we create table first and then declare primary key as alter table? Can we declare primary key inline?


- Alexander Kolbasov


On April 7, 2017, 5:24 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> -----------------------------------------------------------
> 
> (Updated April 7, 2017, 5:24 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
>     https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is associated with more than one path.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/1/
> 
> 
> Testing
> -------
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.


> On April 10, 2017, 5:42 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql
> > Lines 15 (patched)
> > <https://reviews.apache.org/r/58267/diff/2/?file=1687123#file1687123line23>
> >
> >     If we want to store unique "PATH_Name" only once, we need to have three tables: one for "AUTHZ_OBJ_I to AUTHZ_OBJ_NAME", one for "PATH_ID to PATH_NAME", and the third table for relationship "AUTHZ_OBJ_I with PATH_ID"

I was incorrect when I mentioned that this schema would avoid duplicate entries for the same path. Even this approach will have duplicate. We are aware that this mapping can be optimized. We have plan to change it later. For now we need a working version so that we let Hao commit in her changes which are blocking the HA functinoality.


- kalyan kumar


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


On April 10, 2017, 12:44 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> -----------------------------------------------------------
> 
> (Updated April 10, 2017, 12:44 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
>     https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is associated with more than one path.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/2/
> 
> 
> Testing
> -------
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by Na Li <li...@cloudera.com>.

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




sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql
Lines 15 (patched)
<https://reviews.apache.org/r/58267/#comment244393>

    If we want to store unique "PATH_Name" only once, we need to have three tables: one for "AUTHZ_OBJ_I to AUTHZ_OBJ_NAME", one for "PATH_ID to PATH_NAME", and the third table for relationship "AUTHZ_OBJ_I with PATH_ID"


- Na Li


On April 10, 2017, 12:44 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> -----------------------------------------------------------
> 
> (Updated April 10, 2017, 12:44 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
>     https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is associated with more than one path.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/2/
> 
> 
> Testing
> -------
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by Alexander Kolbasov <ak...@gmail.com>.

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


Fix it, then Ship it!




Ship It!


sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 69 (patched)
<https://reviews.apache.org/r/58267/#comment244447>

    Please provide javadoc for this new function
    And, again, the name of this function seems wrong


- Alexander Kolbasov


On April 10, 2017, 12:44 p.m., kalyan kumar kalvagadda wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58267/
> -----------------------------------------------------------
> 
> (Updated April 10, 2017, 12:44 p.m.)
> 
> 
> Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
> 
> 
> Bugs: SENTRY-1629
>     https://issues.apache.org/jira/browse/SENTRY-1629
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.
> 
> Submitted code changes address this issue. Here is the snapshot of the changes
> 
> 1. Change the JDO definition to have Path as separate entity. 
> 2. SQL changes needed for the handle the new JDO definition.
> 3. Application changes to use new JDO definition.
> 4. Updated unit test cases to test the case where an authz object is associated with more than one path.
> 
> 
> Diffs
> -----
> 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704 
>   sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8 
>   sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
>   sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9 
>   sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77 
> 
> 
> Diff: https://reviews.apache.org/r/58267/diff/2/
> 
> 
> Testing
> -------
> 
> Testing done:
> 1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
> 2. Added unit tests to be sure that the actual issues is addressed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

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

(Updated April 11, 2017, 9:19 p.m.)


Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.


Changes
-------

Addressed review comments from sasha


Bugs: SENTRY-1629
    https://issues.apache.org/jira/browse/SENTRY-1629


Repository: sentry


Description
-------

Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.

Submitted code changes address this issue. Here is the snapshot of the changes

1. Change the JDO definition to have Path as separate entity. 
2. SQL changes needed for the handle the new JDO definition.
3. Application changes to use new JDO definition.
4. Updated unit test cases to test the case where an authz object is associated with more than one path.


Diffs (updated)
-----

  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java 56e456eb37dfbbbb6d0a14402f57dbc400be9b72 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java PRE-CREATION 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo e981bcf0f327346c09cdbe5785fb8824fc62e704 
  sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 802b9c6cbf8e9ad015e37037b809b58c956de746 
  sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.derby.sql 1883626262bf4f4936f156a7ac74365b9b5873df 
  sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.mysql.sql 1829e2fa1f02a4339e7af4bf45a169013e9ec65f 
  sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.oracle.sql 7de9751892a8ff84067f67d542ac58d33e9148d8 
  sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-1365.postgres.sql adf5f1f39596309183f8c80d2c8ad1f1a7730236 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql 547bbe8136186658e7fe76ab24934157ea5300ff 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql 6474389a18ea59da28d3d7125cf227c7aaa7f7aa 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql 1ab83432db233ee4e7aa054adc1b82c26248a099 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql 0418b298f7cbd8f430733cb329e9fca263bda0f7 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql 68d2c8d53d7c468269ca2c41986ef8651b94f5c7 
  sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql 5376c166659b3e8e373c8c20818f5b1290af90c9 
  sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java aaa0b9fd30bb68fded67f885af4f77bc71398e77 


Diff: https://reviews.apache.org/r/58267/diff/3/

Changes: https://reviews.apache.org/r/58267/diff/2-3/


Testing
-------

Testing done:
1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
2. Added unit tests to be sure that the actual issues is addressed.


Thanks,

kalyan kumar kalvagadda

Re: Review Request 58267: SENTRY-1629 sql changed needed for MAuthzPathsMapping.

Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.

(Updated April 10, 2017, 12:44 p.m.)

Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.

Changes
-------

Addressed ewview comments from sasha.

Bugs: SENTRY-1629
https://issues.apache.org/jira/browse/SENTRY-1629

Repository: sentry

Description
-------

Older JDO defination had issues handling multiple paths associtaed with one Authorizable object.

Submitted code changes address this issue. Here is the snapshot of the changes

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/58267/diff/2/

Changes: https://reviews.apache.org/r/58267/diff/1-2/

Testing
-------

Testing done:
1. Tested the sql changes with derby, mysql, oracle, postgres and db2.
2. Added unit tests to be sure that the actual issues is addressed.

Thanks,

kalyan kumar kalvagadda