----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58412/#review172301 -----------------------------------------------------------
Initial batch of review comments. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 256 (patched) <https://reviews.apache.org/r/58412/#comment245414> It would be nice to explain the problem in HIVE-15761 in a few words and how this works around it. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 258 (patched) <https://reviews.apache.org/r/58412/#comment245415> Would this work correctly if we have two HMSFollowers running concurrently? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 280 (patched) <https://reviews.apache.org/r/58412/#comment245416> I would print stacktrace here as well. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 438 (patched) <https://reviews.apache.org/r/58412/#comment245422> Here and in other places in this function there isn't any value in throwing an exception. We got something wrong from Hive, we should just log it and continue. Stack trace would add much value. Also here we know that one of the three is null and yet we try to show them all as %s so we are likely to get NPE here instead. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 450 (patched) <https://reviews.apache.org/r/58412/#comment245423> ame comment about exception. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 463 (patched) <https://reviews.apache.org/r/58412/#comment245424> Same comment about exception. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 482 (patched) <https://reviews.apache.org/r/58412/#comment245425> I'd suggest rewriting as if (! sync...) { return } sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 497 (patched) <https://reviews.apache.org/r/58412/#comment245426> Same suggestion for early return sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 56 (patched) <https://reviews.apache.org/r/58412/#comment245405> Here and in other similar places, you may consider using Collections.singletonList(location). If you don't want to use it, consider creating ArrayList with size 1. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 88 (patched) <https://reviews.apache.org/r/58412/#comment245406> You convert authzObj to lower case in addPaths anyway, so this isn't needed sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 176 (patched) <https://reviews.apache.org/r/58412/#comment245408> seems that newAuthzObj is redundand. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 182 (patched) <https://reviews.apache.org/r/58412/#comment245407> you may want to remove dot on the previous line. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 195 (patched) <https://reviews.apache.org/r/58412/#comment245411> new HashSet<>() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 221 (patched) <https://reviews.apache.org/r/58412/#comment245410> remove dot on the previous line sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 234 (patched) <https://reviews.apache.org/r/58412/#comment245409> It is better to use new HashSet<>() here sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 243 (patched) <https://reviews.apache.org/r/58412/#comment245412> Do you want to continue with other paths instead of returning here? Table may contan some partitions which do not have valid HDFS path. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Lines 334 (patched) <https://reviews.apache.org/r/58412/#comment245413> Is there anything to do if oldPathTree is null but newPathTree is not null? - Alexander Kolbasov On April 18, 2017, 9:34 p.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58412/ > ----------------------------------------------------------- > > (Updated April 18, 2017, 9:34 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > SENTRY-1587: Refactor SentryStore transaction to persist a single path > transcation bundled with corresponding delta path change > > Change-Id: Ice77f631e92c0e74b42ae644e1b2f90c6089e62f > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > 90aaaef0e15306627d7108f12a74a29848055c0b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 14e967aa1065f16e8d4c3f61db2f9055959fa9e6 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java > c22364fa26c80415827313f4d26f6c53b71b6f6c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java > c74384688ca920c79fb2987d225042e135cdfd53 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 08520f30f3529af88d7dab9ae3623f28f0e43558 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb13b0d5015aefe892a6f7e46812ea75124 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 87e329588990be129197d598dd1db4487f8e0f25 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > 24ab1a8b392f23bc75759733bef7cecd4bc2ac34 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 7769f24bb4c062016084bcafe7d50a0f0e013824 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 0e97466c9ba6973d8e787819e292c3b826472b38 > > > Diff: https://reviews.apache.org/r/58412/diff/3/ > > > Testing > ------- > > 1. Add new unit test in TestSentryStore > 2. Enabled hdfs sync integration test > > > Thanks, > > Hao Hao > >
