----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68926/#review209276 -----------------------------------------------------------
LGTM! You'll have my ship-it after we chat about the breaking change comment. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java Lines 266 (patched) <https://reviews.apache.org/r/68926/#comment293565> Is it necessary to support this case? If we don't expect null/empty arguments to be used, then preconditions are better here. If due to a bug some code does call this with an uninitialized argument, then there would be an excpetion due to failed precondition. Whereas right now this function will quietly return and allow the grant to proceed. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java Line 229 (original), 225 (patched) <https://reviews.apache.org/r/68926/#comment293571> 1. Since we expect these calls to be marshaled by some impemenetations this we'd better use a Thrift type to return here. Notice how this whole interface uses Thrift types and primitive types, for this very reason. 2. This type of change is a breaking change and it will cause challenges with clusters created before the change tlking to the same metastore. I'll ping you offline with details, but there's the option of adding a new method and deprecating this one to remove it later. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Line 624 (original), 621 (patched) <https://reviews.apache.org/r/68926/#comment293572> An extra space after (. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Line 721 (original), 716 (patched) <https://reviews.apache.org/r/68926/#comment293566> Extra space after (. sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java Line 775 (original), 769 (patched) <https://reviews.apache.org/r/68926/#comment293567> Here and a few places below, there's an extra space after (. - Vasili Zolotov On Oct. 5, 2018, 2:23 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68926/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2018, 2:23 p.m.) > > > Review request for sentry and kalyan kumar kalvagadda. > > > Bugs: SENTRY-2372 > https://issues.apache.org/jira/browse/SENTRY-2372 > > > Repository: sentry > > > Description > ------- > > Make the grant option check on the SentryPolicyStoreProcessor so that it gets > the group information from it. The SentryStore has the new method to check > the grant option in the DB only. > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 709434c388689b63d5db7d71cd6cc47952d647bf > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 1722109e381bc7be81a4673d12c1ac9f81195c47 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java > 3a68eb6bfa0224d1bce17f0bfccb008ab9b291f2 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreUtils.java > PRE-CREATION > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestHMSFollowerSentryStoreIntegration.java > a8868364437ac637ba4a9ad551408de0a9304984 > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > a299e008cc0df92f3d692024c7794aa494b64d2d > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreToAuthorizable.java > 25f94fa05b05abf8c1dbc33e97e5e88ae01794e4 > > > Diff: https://reviews.apache.org/r/68926/diff/2/ > > > Testing > ------- > > > Thanks, > > Sergio Pena > >
