> On Nov. 25, 2015, 6 p.m., Jarek Cecho wrote: > > Thanks for picking this up Dian! I really like how we're making the > > Kerberos integration for our test suite in a way that is compatible with > > making them run on a real cluster, huge +1 from my side! > > > > Couple of general notes: > > > > 1) I don't like how we had to add "throws Exception" to a lot of methods > > that are not suppose to throw Exception (as a class). I can see that it's > > mostly because we have added > > KerberosTestUtils.authenticateWithSqoopServer() to the getClient() method. > > I have note below mentioning that I don't feel that it's right approach, > > but in case that there is no other way, can we perhaps move that logic to > > @Before method rather then adding "throws Exception" everywhere? > > > > 2) I know that the RealKdcRunner currently does nothing and hence it's also > > serving the purpose of "NoKerberosRunner", but can we add such class > > explicitly? I'm pretty sure that once we start testing the RealKdcRunner, > > we will have to add some code inside and will no longer me be > > "NoKerberosRunner", so we should have such runner present.
Hi Jarcec, Thanks a lot for the review comments. Have updated the patch accordingly. One point to note is that currently this patch only covers tests for mini sqoop server. I'd like to add tests for real sqoop server in separate JIRA. Actually although the current patch doesn't cover tests for real sqoop server, the implementation here indeed considered that and so for real sqoop server support we only need to add a RealKdcRunner and a few small changes. - Dian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40635/#review108018 ----------------------------------------------------------- On Dec. 2, 2015, 6:04 a.m., Dian Fu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40635/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2015, 6:04 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2705 > https://issues.apache.org/jira/browse/SQOOP-2705 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > At the first step of providing kerberos support for integration tests, I'd > like to add kerberos support for SqoopMiniCluster make sure the communication > between sqoop client and sqoop server are kerberos enabled. > > > Diffs > ----- > > pom.xml 91721ce > test/pom.xml 4e1e197 > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java > 4c5d3a8 > > test/src/main/java/org/apache/sqoop/test/infrastructure/providers/KdcInfrastructureProvider.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/KdcRunner.java PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/KdcRunnerFactory.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/MiniKdcRunner.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/NoKerberosKdcRunner.java > PRE-CREATION > > test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java > 325a790 > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java > 9ae941f > test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java > bd4ba6a > test/src/main/java/org/apache/sqoop/test/utils/KerberosTestUtils.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/utils/SqoopUtils.java 5964bcd > > test/src/test/java/org/apache/sqoop/integration/server/InformalObjectNameTest.java > 920679f > > test/src/test/java/org/apache/sqoop/integration/server/ShowJobInOrderTest.java > cbf1e90 > > test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java > 9e682bc > test/src/test/java/org/apache/sqoop/integration/server/VersionTest.java > e86a6cc > > test/src/test/java/org/apache/sqoop/integration/server/rest/LinkRestTest.java > 99959ac > test/src/test/java/org/apache/sqoop/integration/server/rest/RestTest.java > 4ac564c > > Diff: https://reviews.apache.org/r/40635/diff/ > > > Testing > ------- > > > Thanks, > > Dian Fu > >
