----------------------------------------------------------- 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 > >