> On Aug. 6, 2015, 4:46 a.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java, > > line 75 > > <https://reviews.apache.org/r/37155/diff/1/?file=1033077#file1033077line75> > > > > why comment out this verification?
Noticed if turn on setMetastoreListener on create and drop there will be active clients still hanging on. I can file a seperate jira to track this issue, but leave this test case uncomment with possible test faiures, put jira into the comments. > On Aug. 6, 2015, 4:46 a.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java, > > line 560 > > <https://reviews.apache.org/r/37155/diff/1/?file=1033081#file1033081line560> > > > > Explain what verification this does. Do we want to just check if the > > actual contains expected or should we validate that they are equal? Intend to check if all items in expected set could be found in returned set; The reason is in case there is test data left from previous test cases. But I can make it more accurate to check if all items in returned set are also found in expected set, or returned set is equavilant to returned set. Just whenever use this method ensure cleanup after method be called. > On Aug. 6, 2015, 4:46 a.m., Lenni Kuff wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestExportImportPrivileges.java, > > line 45 > > <https://reviews.apache.org/r/37155/diff/1/?file=1033083#file1033083line45> > > > > when would we not want to clear the db before/after the test? There are still legacy test cases are dependent on each other; My plan is to clean up them later not to address all such issues in one cr. It requires more efforts. (http://github.mtv.cloudera.com/CDH/sentry/blob/cdh5-1.5.1/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java). Does it sound good? On Aug. 6, 2015, 4:46 a.m., Anne Yu wrote: > > Can you provide a bit more detail in the review description of what the > > actual problem is that you are fixing? Before tests are written in a way like this, Step 1, Create policy, for sentry service it means if database/table etc. does not exist, create them, then create role/grant privileges, write policy file, ((http://github.mtv.cloudera.com/CDH/sentry/blob/c9132a83d061deb8e8b76a8e5f69cd9b5484a6fb/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java, can take a look at addPrivilege function) Step2, tests drop database with tables recreate them again; These steps posed a problem like associated databases/tables privileges are also removed from sentry; So I basically swap the testing order, create database/table then call policy function to create role/grant privileges; It's better test function explicitly create test data (db,table, partition) then do privileges on them. The other part of the cr is to clean up dropDb in the begining of each method by adding a cleanup after/before in the base test class, the chanlenging is some test classes override base one's after/before, so I have to add cleanup explicitly into such test classes. Hope this clarifies more. - Anne ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37155/#review94350 ----------------------------------------------------------- On Aug. 6, 2015, 1:25 a.m., Anne Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37155/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2015, 1:25 a.m.) > > > Review request for sentry and Lenni Kuff. > > > Bugs: SENTRY-834 > https://issues.apache.org/jira/browse/SENTRY-834 > > > Repository: sentry > > > Description > ------- > > Second effort to fix hive e2e test failures on a real cluster test: > TestDbConnections, TestDbExportImportPrivileges, TestDbJDBCInterface > > > Diffs > ----- > > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbConnections.java > 7024263 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbExportImportPrivileges.java > 3d67ab7 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbJDBCInterface.java > 27897f4 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbMetadataObjectRetrieval.java > 53c7d0b > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > 16695f5 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestCrossDbOps.java > 5b1e2b8 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestExportImportPrivileges.java > b9e4da9 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestJDBCInterface.java > 6a9ae5c > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/TestMetadataObjectRetrieval.java > fbfb031 > > Diff: https://reviews.apache.org/r/37155/diff/ > > > Testing > ------- > > https://builds.apache.org/job/PreCommit-SENTRY-Build/759/console > > > Thanks, > > Anne Yu > >
