> On Aug. 1, 2017, 6:40 p.m., kalyan kumar kalvagadda wrote: > > 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/diff/4/?file=1785323#file1785323line249> > > > > When hdfs sync is not enabled, this API should have returned false.
Thanks for finding this. I have fixed it. > On Aug. 1, 2017, 6:40 p.m., kalyan kumar kalvagadda wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 168 (patched) > > <https://reviews.apache.org/r/61047/diff/4/?file=1785324#file1785324line168> > > > > 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. remove two places. Only leave NotificationProcessor since it is used in many places and in those functions, there is no conf saved for this class instance. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61047/#review181906 ----------------------------------------------------------- On Aug. 1, 2017, 9:01 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, 9:01 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/6/ > > > Testing > ------- > > unit tests > > > Thanks, > > Na Li > >
