----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55706/#review168025 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 29 (patched) <https://reviews.apache.org/r/55706/#comment240069> Please add some comments for the class. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java Lines 31 (patched) <https://reviews.apache.org/r/55706/#comment240070> Please add comments for the class. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java Lines 68 (patched) <https://reviews.apache.org/r/55706/#comment240083> You might want to break this line. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java Lines 72 (patched) <https://reviews.apache.org/r/55706/#comment240082> It seems that `tPermUpdate` is using `curSeqNum`. What prevent us to use `curSeqNum` here for `permissionsUpdate`? At lease in this patch, these two seq number are not the same, IIUC? 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/#comment240032> Can we use the interface here? I.e, `private final Map<String, List<String>> roleImage;` as well as the other places in this file. Btw, Could you add comments of these fields, especially, what does each `String` field represent? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2281 (original), 2314 (patched) <https://reviews.apache.org/r/55706/#comment240056> It can be `private`. Btw, it is better to return `Map<String, List<String>>`. Would you consider rename it to `retrieveFullRollImageCore`? It is not executed in a transaction, so it might not suppose to be called outside this class? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 2311 (original), 2364 (patched) <https://reviews.apache.org/r/55706/#comment240042> It should be a private function. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2369 (patched) <https://reviews.apache.org/r/55706/#comment240061> Will `query.execute()` return `null`? http://www.datanucleus.org/javadocs/javax.jdo/3.2/javax/jdo/Query.html#execute-- - Lei Xu On March 2, 2017, 1: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, 1: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 > >
