> On July 24, 2017, 8:57 p.m., Alexander Kolbasov wrote: > > 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/diff/1/?file=1780874#file1780874line171> > > > > The name is a bit strange. From SentryStore point of view why should it > > care about hdfs sync? This is the layer that deals with perssitent storage. > > So this should be a variable that determines whether updates should be > > stored or not.
will rename > On July 24, 2017, 8:57 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Lines 261 (patched) > > <https://reviews.apache.org/r/61047/diff/1/?file=1780874#file1780874line261> > > > > Can we just add a method to SentryStore that sets it and call it from > > SentryService? I will add it as input of constructor > On July 24, 2017, 8:57 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > > Line 724 (original), 727 (patched) > > <https://reviews.apache.org/r/61047/diff/1/?file=1780874#file1780874line727> > > > > There is a lot of duplication - many functions do that. Can you move > > this to a common code - e.g. to the execute() method? If I do it in execute(), it means all callers will create DeltaTransactionBlock() unnecessary if hdfssync is disabled. > On July 24, 2017, 8:57 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 120 (patched) > > <https://reviews.apache.org/r/61047/diff/1/?file=1780875#file1780875line120> > > > > Why do we need these? SentryStore may just compute one boolean and keep > > it rather then keep three separate vars. will update > On July 24, 2017, 8:57 p.m., Alexander Kolbasov wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > > Lines 291 (patched) > > <https://reviews.apache.org/r/61047/diff/1/?file=1780875#file1780875line291> > > > > Why do we need a whole gunction for that? A single boolean should be > > sufficient. It is called in two places. So I want to have a single place to hold that logic. - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61047/#review181257 ----------------------------------------------------------- On July 21, 2017, 7:28 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61047/ > ----------------------------------------------------------- > > (Updated July 21, 2017, 7:28 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/service/persistent/SentryStore.java > 670bc5e > > 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/TestSentryStore.java > a8ebf7c > > > Diff: https://reviews.apache.org/r/61047/diff/1/ > > > Testing > ------- > > unit tests > > > Thanks, > > Na Li > >
