----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25879/#review55484 -----------------------------------------------------------
Thanks for the patch. Overall looks really good, left a few comments let me know if you have any questions. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95852> Finish comment and fix grammar. Could you also add one or two more sentances on what this class does? For example: "Callers will only receive objects back which they have privileges to access and all object lists (ex getAllTables()) will be filtered to exclude items the requetor does not have privileges to access." sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95858> since this is a subclass of ObjectStore consider a name such as "FilteringObjectStore" or "AuthorizingObjectStore". To me, MetastoreFilter sounds like it might be a standalone class used to filter results returned by the metastore. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95854> Since you repeat this message in a bunch of places, consider making it a constant. Also consider re-phrasing to something like: "Table does not exist or insufficient privileges to access: <dbName>.<tableName>" similar comment for the db error messages. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95855> Should this be implemented? If not, remove commented out code. sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95859> which user has so ... sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95860> Instead of doing this initialization for every request or just cache the value and use that for future requests? sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95856> Should this always return true? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreFilter.java <https://reviews.apache.org/r/25879/#comment95857> It might be easier to debug test failures if you split this in to 3 separate test cases. something like: testFullAccess(); testPartialAccess(); testNoAccess(); - Lenni Kuff On Oct. 6, 2014, 1:19 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25879/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2014, 1:19 a.m.) > > > Review request for sentry, Lenni Kuff, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > The Metastore plugin currently enforces Sentry policies for metadata > modifications. This makes it inconsistent with Hive plugin that support > privileges for both metadata read and write. > We should support the policy enforcement for metadata read as well. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreFilter.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/hiveserver/HiveServerFactory.java > 4c66ffe > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/metastore/TestMetastoreFilter.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/25879/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
