> On July 27, 2017, 3:26 p.m., Sergio Pena wrote: > > 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/diff/2/?file=1784346#file1784346line247> > > > > Why not checking if enableSaveDeltaBlock should be saved in this > > constructor instead of passing the new boolean parameter? > > > > I see that before calling this constructor, you're doing this, and then > > passing the same conf parameter to the constructor: > > > > SentryServiceUtil.isHDFSSyncEnabled(conf); > > > > We can get the information necessary from the conf. Btw, if PERM is > > enabled by HDFS disabled, do we want to avoid writing perm updates?
That is what I did. Sasha wants SentryStore to be unaware of such logic. And let caller to decide. That is why I changed many places. If Perm is enabled and HDFS is disabled, we save Perm info into MSentryPrivilege for example, but not save the perm changes (only HDFS uses the perm change, all other use the permission info). > On July 27, 2017, 3:26 p.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Line 169 (original), 175-187 (patched) > > <https://reviews.apache.org/r/61047/diff/2/?file=1784348#file1784348line175> > > > > Can we move this to a method inside SentryServiceUtil? we can - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61047/#review181571 ----------------------------------------------------------- On July 27, 2017, 3:04 a.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:04 a.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/2/ > > > Testing > ------- > > unit tests > > > Thanks, > > Na Li > >
