----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56458/#review164874 -----------------------------------------------------------
Please try to separate SQL changes from java changes into separate changesets. SQL changes are fine. For Java changes - the mysql test looks more like a home directory tool so I would question it inclusion here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentrySchemaTool.java (line 160) <https://reviews.apache.org/r/56458/#comment236713> Why do you need this method? Who is using it? Is it only for testing or actually used for something? If this is testing-only, please move it to the test. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentrySchemaTool.java (line 165) <https://reviews.apache.org/r/56458/#comment236711> Is this a stray non-empty line? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaTool.java <https://reviews.apache.org/r/56458/#comment236715> Why is this removed? It is never used or there is another reason? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java (line 30) <https://reviews.apache.org/r/56458/#comment236716> Is this test included in automatic run or it should be run manually? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java (line 36) <https://reviews.apache.org/r/56458/#comment236717> Please format using javadoc conventions - use <p> for paragraph separation and {@code} for code snippets. Where should these dependencies be added? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java (line 40) <https://reviews.apache.org/r/56458/#comment236718> lines 40 and 41 should probably be removed sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java (line 46) <https://reviews.apache.org/r/56458/#comment236719> Do we need to change it when we are running the test? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java (line 55) <https://reviews.apache.org/r/56458/#comment236720> Should we change this manually every time? Please do not use your name or "cloudera" as a pass. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java (line 58) <https://reviews.apache.org/r/56458/#comment236721> What is dot in this path? WIll this work if we run test from a different dir? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java (line 66) <https://reviews.apache.org/r/56458/#comment236722> What is this IP address? - Alexander Kolbasov On Feb. 8, 2017, 7:16 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56458/ > ----------------------------------------------------------- > > (Updated Feb. 8, 2017, 7:16 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Vamsee Yarlagadda, > and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > SENTRY-1614 Removed code having PATHS column as primary key. Primary key of > this long is not accepted by all databases > > Code changes include following > 1. Removing PATHS column from primary key as Primary key of this long is not > accepted by all databases. > 2. Remove the index on MAUTHZPATHSMAPPING_PATHS. As there would be implicit > index created with AUTHZ_OBJ_ID_OID. > 3. Added test to run Schematool tests with mysql. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentrySchemaTool.java > d75e24bb17221a7b676e735d9976e83d8f17c182 > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.derby.sql > 1883626262bf4f4936f156a7ac74365b9b5873df > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.mysql.sql > 1829e2fa1f02a4339e7af4bf45a169013e9ec65f > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.oracle.sql > 7de9751892a8ff84067f67d542ac58d33e9148d8 > > sentry-provider/sentry-provider-db/src/main/resources/007-SENTRY-872.postgres.sql > adf5f1f39596309183f8c80d2c8ad1f1a7730236 > sentry-provider/sentry-provider-db/src/main/resources/sentry-db2-1.8.0.sql > 0606116a1d31c400fb6dadcd80a2284007117ab2 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-derby-1.8.0.sql > be9a33e6b0904b7c8ec522906d967081631108d6 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-mysql-1.8.0.sql > 1c8848c30d0432ebdbd18b73a1d27c61d2b7bcae > > sentry-provider/sentry-provider-db/src/main/resources/sentry-oracle-1.8.0.sql > fc7b53f41e97fad5e5baeb254b686a0e8cc5b003 > > sentry-provider/sentry-provider-db/src/main/resources/sentry-postgres-1.8.0.sql > ce807a5aee9d04e119dde32784cbe958bf933feb > > sentry-provider/sentry-provider-db/src/main/resources/sentry-upgrade-db2-1.7.0-to-1.8.0.sql > 92d0e3314141e01cab3a8321bba0ca253378b27f > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaTool.java > 68abf277e0eab48a16f9c1b10147a11b9d10394a > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/tools/TestSentrySchemaToolWithmysql.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/56458/diff/ > > > Testing > ------- > > 1. ran the sql scripts with all the databases we support(db2, postgress, > oracle and mysql) > 2. Ran TestSentrySchemaTool tests. > > > Thanks, > > kalyan kumar kalvagadda > >
