Re: Review Request 69212: SENTRY-2329: Integrate sentry with Hadoop 3.1.1

2018-11-01 Thread Na Li via Review Board 

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
Line 143 (original)


why do you remove it? Is it impossible to throw this exception?


- Na Li


On Oct. 30, 2018, 2:16 a.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69212/
> ---
> 
> (Updated Oct. 30, 2018, 2:16 a.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2329
> https://issues.apache.org/jira/browse/sentry-2329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump hadoop.version to 3.1.1. The changes on Sentry are compatible with 
> Hadoop 2.7.
> 
> 
> Diffs
> -
> 
>   pom.xml acbdcc2722bf189811cb528ac1b2d07983a571c2 
>   sentry-binding/sentry-binding-solr/pom.xml 
> f2a5fca76d3d220fcf2b72a3179ff5218fc6577c 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> df6f04c048b502ff5f8e8ec397d75166faba8c3c 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  c9ecc4021b167d98c7dade409c97ae7d26e967ea 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  18b62652a11dfee6683cb8f24944ccd3d344dc9f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithKerberos.java
>  5d94d4bc6a2a47189e69556a5e4d9bdee05952a7 
> 
> 
> Diff: https://reviews.apache.org/r/69212/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69212: SENTRY-2329: Integrate sentry with Hadoop 3.1.1

2018-11-01 Thread kalyan kumar kalvagadda via Review Board 

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




pom.xml
Lines 216-231 (patched)


I think hadoop-minicluster can be made a test only dependency as it is used 
only for testing by changing the scope to test.

If we are good with this we don't have to exclude these as these 
dependencies are not propogated anyway.



sentry-binding/sentry-binding-solr/pom.xml
Lines 32-47 (patched)


Can you elaborate on this?

If the scope of hadoop-minicluster is changed to test, would there stil be 
issue with the jetty version?



sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
Lines 334-338 (patched)


What made you add this check?

Was there something that was failing OR Just added for safety?


- kalyan kumar kalvagadda


On Oct. 30, 2018, 2:16 a.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69212/
> ---
> 
> (Updated Oct. 30, 2018, 2:16 a.m.)
> 
> 
> Review request for sentry and kalyan kumar kalvagadda.
> 
> 
> Bugs: sentry-2329
> https://issues.apache.org/jira/browse/sentry-2329
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Bump hadoop.version to 3.1.1. The changes on Sentry are compatible with 
> Hadoop 2.7.
> 
> 
> Diffs
> -
> 
>   pom.xml acbdcc2722bf189811cb528ac1b2d07983a571c2 
>   sentry-binding/sentry-binding-solr/pom.xml 
> f2a5fca76d3d220fcf2b72a3179ff5218fc6577c 
>   sentry-hdfs/sentry-hdfs-common/pom.xml 
> df6f04c048b502ff5f8e8ec397d75166faba8c3c 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  c9ecc4021b167d98c7dade409c97ae7d26e967ea 
>   
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryINodeAttributesProvider.java
>  18b62652a11dfee6683cb8f24944ccd3d344dc9f 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryWebServerWithKerberos.java
>  5d94d4bc6a2a47189e69556a5e4d9bdee05952a7 
> 
> 
> Diff: https://reviews.apache.org/r/69212/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 69087: SENTRY-2249: Enable batch insert of HMS paths in Full Snapshot.

2018-11-01 Thread Sergio Pena via Review Board 

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




sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Lines 37 (patched)


This is already defined in the package.jdo. Does it need to be defined here 
as well? My understanding is that it can be either on the package.jdo or the 
class file or both, but should we be consistent where we put these declarations?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
Line 36 (original), 42 (patched)


Is this variable needed now that you remove the getters and setters?

Btw, why isn't this variable used anymore?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 259-260 (original), 259-260 (patched)


There are weird characters at the end of these lines.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 259-261 (original), 259-261 (patched)


What is the strategy for the ID? Will it continue using the increment 
strategy?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 300 (patched)


Why a new class that points to the same table as MPath is needed?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 301 (patched)


with our?



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 303-311 (patched)


There are weird characters at the end of these lines.



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
Lines 303-311 (patched)


There are weird characters at the end of these lines.


- Sergio Pena


On Oct. 31, 2018, 11:28 p.m., kalyan kumar kalvagadda wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69087/
> ---
> 
> (Updated Oct. 31, 2018, 11:28 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena.
> 
> 
> Bugs: SENTRY-2249
> https://issues.apache.org/jira/browse/SENTRY-2249
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently each entry in full snapshot of HMS is persisted one entry at a 
> time. Instead it could be optimized by persisting the path entries in 
> batches. DB operations are expensive, reducing the number of database 
> operations and around trip time will help. This would decrease the time to 
> persist the snapshot in to database significantly.
> 
> Size of the batch could be configurable.
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java
>  d8c1061d36ed0b92116f0b2dd5ec820ccb166818 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  092060c450c6a906850630cb10454737157af5fe 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java
>  c51f25a0393b482814afcd3b7a19e547b689ac6e 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
>  b0eaff2120a80685da07c65a7706edf2be62ee01 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/MPathToPersist.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo
>  20ec0deab6b97065cfe99beea3d14a6c7268aac3 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  33c40613a05f7c7fde314af6aba6b269bf6ffaae 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  66db6ae9a436b9728fb3c2ebdd21167ef042f937 
> 
> 
> Diff: https://reviews.apache.org/r/69087/diff/3/
> 
> 
> Testing
> ---
> 
> Made sure all the unit tests passed.
> 
> 
> Thanks,
> 
> kalyan kumar kalvagadda
> 
>