----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55706/#review164467 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java (line 23) <https://reviews.apache.org/r/55706/#comment236206> This is a very vague interface. The only supported method (if you ignore the name) is a map from long ID to any object of type K, so it is way too generic and has nothing to do with image retrieval. Can we restrict K to be at least a class extending Update? It seems that it would be good to consider having Full Update as a subclass of Update, but this can be done outside the scope of this change. In any case please provide more documentation of what implementors of this interface are supposed to actually implement. sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java (line 32) <https://reviews.apache.org/r/55706/#comment236218> What is the semantics of this call in terms of seqNum? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 157) <https://reviews.apache.org/r/55706/#comment236212> These seem to be very generic - can we have them in some common Sentry code? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 160) <https://reviews.apache.org/r/55706/#comment236208> What is paths? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 161) <https://reviews.apache.org/r/55706/#comment236209> What does it return? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 163) <https://reviews.apache.org/r/55706/#comment236207> Can we relax the arg to be Iterable? Also name - something like joinPath or pathJoin seems to be better sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 169) <https://reviews.apache.org/r/55706/#comment236210> What is path? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java (line 171) <https://reviews.apache.org/r/55706/#comment236211> What does it return? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java (line 38) <https://reviews.apache.org/r/55706/#comment236215> With the current design - do we support retrieving full image with any seqNum? Why do we need to pass it to the retriever? Aren't we going to retrieve the latest snapshot? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java (line 42) <https://reviews.apache.org/r/55706/#comment236213> There is already call to time() above in the try statement sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java (line 43) <https://reviews.apache.org/r/55706/#comment236214> May be just // Read the most up-to-date snapshot of Sentry paths information. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java (line 46) <https://reviews.apache.org/r/55706/#comment236216> This isn't used anywhere - but may be it should be used on line 51? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java (line 51) <https://reviews.apache.org/r/55706/#comment236217> should it be seqNum or curSeqNum? This is mentioned in TODO, so I guess this is the plan? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java (line 44) <https://reviews.apache.org/r/55706/#comment236220> This is alreday done on line 42 sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java (line 46) <https://reviews.apache.org/r/55706/#comment236221> See comment for PathImageRetriever sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java (line 56) <https://reviews.apache.org/r/55706/#comment236222> Why do we construct it with curSeqNum but later in line 75 change it to seqNum? - Alexander Kolbasov On Feb. 3, 2017, 7:53 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55706/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2017, 7:53 a.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 > ffb07560d7c40fb14cef77950012c2c81fa3fd28 > > 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 > 9b54db171d531d4a4d1335aa93a3d3122d55ce9b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > dfaac1555fe729efab3f8fdccb914978001fee56 > > Diff: https://reviews.apache.org/r/55706/diff/ > > > Testing > ------- > > > Thanks, > > Hao Hao > >
