> On June 19, 2018, 10:38 p.m., Sergio Pena wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > > Lines 1386 (patched) > > <https://reviews.apache.org/r/67649/diff/3/?file=2042471#file2042471line1386> > > > > This line has an unecessary extra ()
will fix > On June 19, 2018, 10:38 p.m., Sergio Pena wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > > Lines 1402-1415 (patched) > > <https://reviews.apache.org/r/67649/diff/3/?file=2042471#file2042471line1402> > > > > We need to catch the same exceptions that sentry_sync_notifications() > > throws. > > > > Btw, if we catch a sync exception, then are we going to ignore the > > ownership grant? will address it > On June 19, 2018, 10:38 p.m., Sergio Pena wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > > Lines 1453 (patched) > > <https://reviews.apache.org/r/67649/diff/3/?file=2042471#file2042471line1453> > > > > 'hive' user should not be hard-coded. Find a way to obtain the user > > that the Sentry server is using instead. Sentry is a service so there will neither be a user OR group for sentry. Actually there is no validation needed for the grantor here as permission is granted by the service but not by the user it self. The option i picked is hard coding the user to hive service user. You are right there is no guarentee that Hive is going to be the service user for hive service. Here is what i think the options are 1. Ignore checking the permissions of the grantor for owner privilege. 2. User sentry service principle as grantor and add code to authorize sentry service principle to grant privileges. 3. Other option is to add new API that does not consider granter and use it only for owner privileges. I prefer option-3 as there is no real validation that we need to make as the the API is invoked by sentry service. > On June 19, 2018, 10:38 p.m., Sergio Pena wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > > Lines 70 (patched) > > <https://reviews.apache.org/r/67649/diff/3/?file=2042472#file2042472line72> > > > > Is this a compatible change? I see that implementations of this > > interface can be configured in the sentry-site.xml. Should we keep the > > method compatibility? > > > > Same question for the next method. Yes, you are right. implementation of this interface can be provided as a configuration. It is not implemented by sentry component only. If this interface is implemented by other components then it would not be a compatible change. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67649/#review205033 ----------------------------------------------------------- On June 19, 2018, 3:50 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67649/ > ----------------------------------------------------------- > > (Updated June 19, 2018, 3:50 p.m.) > > > Review request for sentry, Na Li and Sergio Pena. > > > Bugs: SENTRY-2241 > https://issues.apache.org/jira/browse/SENTRY-2241 > > > Repository: sentry > > > Description > ------- > > Sentry has SentrySyncHMSNotificationsPostEventListener which is added a post > listener in HMS. This listener should be extended to get the owner > information of tables and databases. Based on these notifications owner > privileges are granted/revoked. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/SentryOwnerInfo.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > b5e01e4c21473523b494cc624318b73ec5732408 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java > 5424bff67bc31d3f4ed2300531706e062f82b9ae > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 7f97ff7f054f797757aac94e243acdc5ee1127a2 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > 52f25dc15d5b6d5e3ef1caa02baec8ed2923e290 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/CounterWait.java > d8c82970b56b1599a07f0e26edab8ed3d59b9948 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f0e373a9aa5342d1b507e8b192cfdbc444242227 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java > 6bfe872320d255acdb10f69e09c19623c04d8951 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > c056446262ddcf61db307eafbb785eac42973c80 > > > Diff: https://reviews.apache.org/r/67649/diff/3/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
