----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23600/#review48400 -----------------------------------------------------------
Thanks for the patch Arun! Mostly looks good, some comments below. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/23600/#comment84961> I do not think serverName can ever be null? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java <https://reviews.apache.org/r/23600/#comment84962> why are we replacing nulls with empty strings, is it because NULL != NULL in some dbs? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/23600/#comment84980> curious, why you had to change this? Not part of this patch but also the scope should be lower cased right? sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql <https://reviews.apache.org/r/23600/#comment84983> Wonder why are some in quotes? Are they system keywords? http://www-01.ibm.com/support/docview.wss?uid=swg21164500 sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.4.0.sql <https://reviews.apache.org/r/23600/#comment84985> This might lead to problem as URI(4000) is too big to be part of unique key. sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/23600/#comment84992> We should probably not provide a default for serverName? We should make serverName strictly required. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/23600/#comment84993> Nit: Can we remove this commented out line? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java <https://reviews.apache.org/r/23600/#comment84995> Nit: Remove debugging help? - Sravya Tirukkovalur On July 17, 2014, 1:47 a.m., Arun Suresh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23600/ > ----------------------------------------------------------- > > (Updated July 17, 2014, 1:47 a.m.) > > > Review request for sentry, Jarek Cecho, Prasad Mujumdar, and Sravya > Tirukkovalur. > > > Repository: sentry > > > Description > ------- > > SENTRY-339 : > > Remove the need to construct a privilegeName to check for uniqueness. > - Remove the PrivilegeName column > - Remove the constructPrivilegeName() method > - Updated the sql scripts > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPrivilege.java > f8491db > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > 945227e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > ff8acdc > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 3bb7285 > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.4.0.sql > f2a62d2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.4.0.sql > f2a62d2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.4.0.sql > 70f4dbb > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.4.0.sql > 363590e > > sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.4.0.sql > 5dfae03 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > fdc7b9c > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 7637376 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > a4ae291 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestPolicyImport.java > 948b0c4 > > Diff: https://reviews.apache.org/r/23600/diff/ > > > Testing > ------- > > Ran all e2e tests and unit tests. > > > Thanks, > > Arun Suresh > >
