----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61047/#review181612 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java Line 68 (original), 69 (patched) <https://reviews.apache.org/r/61047/#comment257203> This file deals with Generic permissions model which is never subject to HDFS sync. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 78 (patched) <https://reviews.apache.org/r/61047/#comment257211> This is unused import sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 171 (patched) <https://reviews.apache.org/r/61047/#comment257214> This is a very confusing name What is saveDeltaBlockEnabled? It should be at least documented, but a better name would be good. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 245 (original), 247 (patched) <https://reviews.apache.org/r/61047/#comment257212> I thnik we should have a simple constructor with old signature for all users who don't care about this. Also, enableSaveDeltaBlock is rather bad name - what is "SaveDeltaBlock"? You never explain what it is. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 4089 (original), 4090 (patched) <https://reviews.apache.org/r/61047/#comment257215> Please update the comment to reflect the changed behavior. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 4096 (original), 4097 (patched) <https://reviews.apache.org/r/61047/#comment257216> This is outside the scope but since you are touching the function - it makes sense to change to tbs = new ArrayList(2); otherwise we allocate space for 10 elements. Not a big deal, but still... sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 4098 (original), 4101 (patched) <https://reviews.apache.org/r/61047/#comment257217> Why not just `tbs.add(new DeltaTransactionBlock(update));` sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java Line 117 (original), 118 (patched) <https://reviews.apache.org/r/61047/#comment257204> WHy have it here? For the purpose of this servlet it doesn't matter whether HDFS sync is enabled or not. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 283 (patched) <https://reviews.apache.org/r/61047/#comment257205> This function isn't needed, instead you need a boolean (that replaces hdfsSyncEnabled and syncPolicyStore) that is true when HMSFollower is enabled. hdfsSYncEnabled and syncPolicyStore are only used to compute this, so they can be local variables instead. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java Lines 225 (patched) <https://reviews.apache.org/r/61047/#comment257210> typo: policy, not pollicy - Alexander Kolbasov On July 27, 2017, 3:57 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61047/ > ----------------------------------------------------------- > > (Updated July 27, 2017, 3:57 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, > Sergio Pena, and Vamsee Yarlagadda. > > > Repository: sentry > > > Description > ------- > > Currently, Sentry does not start HMSFollower when HDFS sync is disabled. This > breaks PERM sync. For example, when a table/database is dropped, its > permission should be removed when perm sync is enabled. Without HMSFollower, > Sentry does not know when table/database is dropped, and therefore, cannot > remove the permission accordingly. > Sentry needs to make updates to have the follow behavior > 1) When HDFS sync is disabled + PERM sync is disabled, do not start > HMSFollower > 2) when HDFS sync is enabled or PERM sync is enabled, start HMSFollower > (Sentry change). > 3) when HDFS sync is enabled, we save path change and perm change. nothing > more. > 4) when perm sync is enabled, we update perm when table/database is created, > dropped or altered, nothing more. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > 4cb46ab > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 670bc5e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryAdminServlet.java > 8a8bbd3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 10d55dc > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceUtil.java > 5826766 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > 82f600b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > a8ebf7c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > 1c3a4f2 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java > a8e8a03 > > > Diff: https://reviews.apache.org/r/61047/diff/3/ > > > Testing > ------- > > unit tests > > > Thanks, > > Na Li > >
