----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25808/#review64852 -----------------------------------------------------------
Ship it! Thanks for the update Guoquan! Patch looks good to me, some minor comments below. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java <https://reviews.apache.org/r/25808/#comment107646> Why serviceName if authorizable = null? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java <https://reviews.apache.org/r/25808/#comment107648> We should probably never have null values, as we will no more have fixed column schema? In which case we can have a null check here instead of iterating over all elements again in checkStrictHeirachy()? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java <https://reviews.apache.org/r/25808/#comment107647> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java <https://reviews.apache.org/r/25808/#comment107649> Case sensitive comparison might be bad only for privilege data objects isnt it? Like table name tb1 and TB1 might be different in some systems where as it might be same in some. But for Sentry constants like action, it is probably fine with ignoring case? As we want SELECT and select to mean the same? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryGMPrivilege.java <https://reviews.apache.org/r/25808/#comment108103> Nit: Update the failure message, "Empty value is not allowed in a strict heirarchy"? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryGMPrivilege.java <https://reviews.apache.org/r/25808/#comment108135> Nit: Might be easy to follow test logic if it was just named serverPrivilege/ collectionPrivilege/ fieldPrivilege? And you can also get rid of existPrivilege2/3 and requestPrivilege3 in that case. - Sravya Tirukkovalur On Dec. 5, 2014, 1:10 p.m., shen guoquan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25808/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2014, 1:10 p.m.) > > > Review request for sentry, Arun Suresh, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > Currently the metadata infrastructure in Sentry only supports database > structure authorize model such as Hive/Impala.In order to support other > no-database structure component like Solr, it needs to extend the metadata > infrastructure. Considering the compatibility and not affect the previous > functionality, it is better to add a new privilege in metadata rather than > modifying the original privilege. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryGMPrivilege.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > bca9fb9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 594201f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryGMPrivilege.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestSentryRole.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/25808/diff/ > > > Testing > ------- > > > Thanks, > > shen guoquan > >
