----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61047/#review181906 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java Line 245 (original), 249 (patched) <https://reviews.apache.org/r/61047/#comment257708> When hdfs sync is not enabled, this API should have returned false. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 73-75 (patched) <https://reviews.apache.org/r/61047/#comment257700> these imports are not used. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 168 (patched) <https://reviews.apache.org/r/61047/#comment257712> Currently, result of SentryServiceUtil.isHDFSSyncEnabled is stored in 4-5 different locations. Instead, it can be stored in only location and can be re-used in all the places. Functinally this is not issue but it would much cleaner and simpler if the value is centralized in one place. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java Lines 170 (patched) <https://reviews.apache.org/r/61047/#comment257710> I feel explicitly setting value is not correct. When sentry store is constructed, configuration is provided to it and it has all the information to decide for it self. Whether the updates are to be persisted or not is an implementation detail of sentry store. I don't see any need to ex[plicity expose an API to set this value. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Lines 129 (patched) <https://reviews.apache.org/r/61047/#comment257713> it is better to change the variable name to hdfsSyncEnabled. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Lines 129 (patched) <https://reviews.apache.org/r/61047/#comment257717> it is better to change the variable name to hdfsSyncEnabled. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java Lines 83 (patched) <https://reviews.apache.org/r/61047/#comment257716> it is better to change the variable name to hdfsSyncEnabled. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java Lines 57 (patched) <https://reviews.apache.org/r/61047/#comment257715> it is better to change the variable name to hdfsSyncEnabled. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java Lines 84 (patched) <https://reviews.apache.org/r/61047/#comment257714> it is better to change the variable name to hdfsSyncEnabled. - kalyan kumar kalvagadda On Aug. 1, 2017, 6:07 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61047/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2017, 6:07 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 > f4842a9 > > 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/HMSFollower.java > 9e8e0e7 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > f631869 > > 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 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > 9b31b3c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestNotificationProcessor.java > c6c9448 > > > Diff: https://reviews.apache.org/r/61047/diff/5/ > > > Testing > ------- > > unit tests > > > Thanks, > > Na Li > >
