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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java
Lines 23 (patched)
<https://reviews.apache.org/r/55706/#comment240436>

    Style: The first sentense shouldn't use links, but may use {@code}.



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 25 (patched)
<https://reviews.apache.org/r/55706/#comment240438>

    hashmap is an unused import



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 45 (patched)
<https://reviews.apache.org/r/55706/#comment240437>

    curSeqnum is never used is it needed?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
Lines 50 (patched)
<https://reviews.apache.org/r/55706/#comment240440>

    Please add a comment, explaining what the code below is doing



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
Lines 35 (patched)
<https://reviews.apache.org/r/55706/#comment240441>

    can be package-private



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
Lines 50 (patched)
<https://reviews.apache.org/r/55706/#comment240447>

    Here and in all the places below it should be List instead of LinkedList



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
Lines 58 (patched)
<https://reviews.apache.org/r/55706/#comment240451>

    Use Map instead of HashMap here and below. HashMap should only be used for 
instance creation



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
Lines 73 (patched)
<https://reviews.apache.org/r/55706/#comment240442>

    Can we pass seqNum in constructor above?



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Line 147 (original)
<https://reviews.apache.org/r/55706/#comment240452>

    Use Map instead of HashMap



sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
Line 151 (original)
<https://reviews.apache.org/r/55706/#comment240448>

    should be List instead of LinkedList



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java
Lines 25 (patched)
<https://reviews.apache.org/r/55706/#comment240445>

    s/of/for



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PermissionsImage.java
Lines 21 (patched)
<https://reviews.apache.org/r/55706/#comment240453>

    Use Map instead of HashMap everywhere here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PermissionsImage.java
Lines 26 (patched)
<https://reviews.apache.org/r/55706/#comment240446>

    s/of/for



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PermissionsImage.java
Lines 32 (patched)
<https://reviews.apache.org/r/55706/#comment240449>

    Indeed, it is better to use List rather then LinkedList



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PermissionsImage.java
Lines 33 (patched)
<https://reviews.apache.org/r/55706/#comment240450>

    Please document what is the structure of the map - what is the String and 
what is the internal map.
    
    Also, please use Map rather than HashMap here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2237 (patched)
<https://reviews.apache.org/r/55706/#comment240454>

    You don't need to document what is called internally in the javadoc (you 
are very welcome to document this in implementation comments). But it is a good 
idea to say what is a snapshot - that it consists of full privilege and role 
images.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2253 (patched)
<https://reviews.apache.org/r/55706/#comment240455>

    Use List here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2254 (patched)
<https://reviews.apache.org/r/55706/#comment240456>

    Use Map here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2267 (patched)
<https://reviews.apache.org/r/55706/#comment240457>

    Use Map here
    Also, since this is a map from String to String please document what is the 
key and the value



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2240 (original), 2271 (patched)
<https://reviews.apache.org/r/55706/#comment240458>

    Use Map on the left



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2292 (original), 2323 (patched)
<https://reviews.apache.org/r/55706/#comment240459>

    Better to use List here



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2338 (patched)
<https://reviews.apache.org/r/55706/#comment240460>

    End first sentence with dot.



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2339 (patched)
<https://reviews.apache.org/r/55706/#comment240461>

    Instead of telling what it calls internally, it is better to explain what 
is done/returned



sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2319 (original), 2367 (patched)
<https://reviews.apache.org/r/55706/#comment240462>

    Here Iterable can be used instead of List


- Alexander Kolbasov


On March 2, 2017, 9:43 p.m., Hao Hao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55706/
> -----------------------------------------------------------
> 
> (Updated March 2, 2017, 9:43 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> Read full permission and path snapshot from SentryDB and make the update 
> available for NN plugin upon requests.
> In detail:
> 1. Added Path/PermissionImage classes to represent corresponding 
> Path/Permission snapshot read from DB.
> 2. Refactor full snapshot retriever APIs in SentryStore to become a single 
> transaction to ensure snapshot consistency.
> 3. Added Path/PermissionImageRetriever classes to retrieve 
> Path/PermissionImage from DB and convert to corresponding 
> Path/PermissionUpdate, which later would be consumed by NN plugin.
> 
> 
> Diffs
> -----
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java
>  992c8b731810297be847e3b802698332aefe8f8f 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java
>  PRE-CREATION 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java
>  e68c7080cec007e3cdf2c89efb9701eef9cca0a0 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java
>  f68f690436301c79e93b0742996dda25a750c0c3 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java
>  6d5c607273bb08597780b655d7b59cd41f0844bb 
>   
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java
>  fe2baa6a446874185e8344bb16d76d803826d1f3 
>   
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java
>  0f0d0a743d5a5f9f1678ac7c5217b9a27537e85b 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PermissionsImage.java
>  PRE-CREATION 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
>  c1186ba405a05be70336e9169a1454208df016ca 
>   
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
>  91f15c0f3999a25a20dfb45f2f28abbda54366f4 
> 
> 
> Diff: https://reviews.apache.org/r/55706/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Hao Hao
> 
>

Reply via email to