----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57232/#review168057 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java Lines 35 (patched) <https://reviews.apache.org/r/57232/#comment240105> If we retrieve `Perm` and `Path` separately, would that be a case that the deltas from them are not consistent, because the `latestChangeID` might be different in them? For example, if we do the following sequence: * Retrieve path deltas. * Create table * Grant permissions on the table. * Retrieve perm deltas. In such order, would some perm deltas can not be applied to the path, as they do not exist on NN yet? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 59 (patched) <https://reviews.apache.org/r/57232/#comment240119> slf4j supports nice parameterized message. It'd be better to use that instead of concating strings. https://www.slf4j.org/faq.html#logging_performance sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3248 (patched) <https://reviews.apache.org/r/57232/#comment240131> What about if `changes.size() == 0`? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3263 (patched) <https://reviews.apache.org/r/57232/#comment240134> Can we change the function name? `findFoo` sounds like returnning a `Foo` object ? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3334 (patched) <https://reviews.apache.org/r/57232/#comment240141> As mentioned in the other comment, is it possible to be `null`? Did that occur to you? http://www.datanucleus.org/javadocs/javax.jdo/3.2/javax/jdo/Query.html#execute-- sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3349 (patched) <https://reviews.apache.org/r/57232/#comment240135> Please add 2 spaces before `throws Exception`. - Lei Xu On March 1, 2017, 11:39 p.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57232/ > ----------------------------------------------------------- > > (Updated March 1, 2017, 11:39 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Add the logic for propagating Sentry Permissions or Sentry representation of > HMS Paths, from a persistent storage instead of in memoery cache of Sentry > service, to a Sentry consumer such as HDFS NameNode. It includes: > 1. Delta update retrieval logic from persistent storage. > 2. Propagation logic for consumer to get the delta update or a complete > snapshot. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java > PRE-CREATION > > 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/ThriftSerializer.java > 69aa098bc71362b1aba0ad219483cfce7d389964 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > 085971b34e3901b7a1d59bd8e7516b25f81ca872 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java > PRE-CREATION > > 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/PermDeltaRetriever.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/SentryHDFSServiceProcessor.java > e4f3f580ee0a6bf83d5a32001bff130c0bff13aa > > 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/SentryStore.java > c1186ba405a05be70336e9169a1454208df016ca > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 91f15c0f3999a25a20dfb45f2f28abbda54366f4 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > f2d74bfe5e413ea88c3c77ef303f1c7f9ee16253 > > > Diff: https://reviews.apache.org/r/57232/diff/2/ > > > Testing > ------- > > > Thanks, > > Hao Hao > >
