You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Abraham Elmahrek <ab...@cloudera.com> on 2015/04/18 00:48:42 UTC
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/
-----------------------------------------------------------
(Updated April 17, 2015, 10:48 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1952
https://issues.apache.org/jira/browse/SQOOP-1952
Repository: sqoop-sqoop2
Description
-------
commit d489e803cc01127ded5270bbe7cf640e674648aa
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Mar 6 17:15:06 2015 -0800
SQOOP-1952
:100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
:100644 100644 32afc77... 1b30782... M pom.xml
:100644 100644 f743d25... d2761f6... M test/pom.xml
:100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
:000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
Diffs (updated)
-----
pom.xml c608ca7
test/pom.xml d8fbfa2
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
test/src/test/resources/suite.xml PRE-CREATION
Diff: https://reviews.apache.org/r/31824/diff/
Testing
-------
Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
Thanks,
Abraham Elmahrek
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
Posted by Jarek Cecho <ja...@apache.org>.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > Hi Abe,
> > thank you very much for working on this one! I have several questions/suggestions inline and a high level questions:
> >
> > It seems that when running integration tests, all that user sees is this output on the console:
> >
> > Running TestSuite
> > Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 905.703 sec
> >
> > I'm wondering if there is a way how to report more granual progress then "working" and "done"? Like "Executed 5 from 26 tests" or something similar? I'm a bit concerned that this is step back in usability as don't have easy way to see what are the tests doing - we would have to wait entire time for them to finish to even noticed that something is failing.
>
> Abraham Elmahrek wrote:
> Yeah good point. I can dig a bit to find out what we can do here.
>
> Jarek Cecho wrote:
> I think that it's not "absolute must have" if this will help us to speed up the tests, but I think that it would be quite desirable :)
>
> Abraham Elmahrek wrote:
> I don't think there's a built in way to do this. I can create a follow up Jira to add a custom listener for this.
>
> It is a stepback in usability in some sense. This patch should improve test speeds at the very least.
I'm personally fine with fixing it in subsequent JIRA and not block this one - as long as we will get the speed up :)
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/#review80876
-----------------------------------------------------------
On April 19, 2015, 9:02 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31824/
> -----------------------------------------------------------
>
> (Updated April 19, 2015, 9:02 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1952
> https://issues.apache.org/jira/browse/SQOOP-1952
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit d489e803cc01127ded5270bbe7cf640e674648aa
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Mar 6 17:15:06 2015 -0800
>
> SQOOP-1952
>
> :100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
> :100644 100644 32afc77... 1b30782... M pom.xml
> :100644 100644 f743d25... d2761f6... M test/pom.xml
> :100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
> :000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
>
>
> Diffs
> -----
>
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java b26ca74
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java 09ddcc7
> pom.xml c608ca7
> test/pom.xml d8fbfa2
> test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java 020fa3f
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
> test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java 9aa69ed
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
> test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java 37a0a7d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java 88db2f2
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java 92a52b8
> test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java 5c90501
> test/src/test/resources/suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31824/diff/
>
>
> Testing
> -------
>
> Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
Posted by Jarek Cecho <ja...@apache.org>.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > Hi Abe,
> > thank you very much for working on this one! I have several questions/suggestions inline and a high level questions:
> >
> > It seems that when running integration tests, all that user sees is this output on the console:
> >
> > Running TestSuite
> > Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 905.703 sec
> >
> > I'm wondering if there is a way how to report more granual progress then "working" and "done"? Like "Executed 5 from 26 tests" or something similar? I'm a bit concerned that this is step back in usability as don't have easy way to see what are the tests doing - we would have to wait entire time for them to finish to even noticed that something is failing.
>
> Abraham Elmahrek wrote:
> Yeah good point. I can dig a bit to find out what we can do here.
I think that it's not "absolute must have" if this will help us to speed up the tests, but I think that it would be quite desirable :)
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java, lines 82-94
> > <https://reviews.apache.org/r/31824/diff/3/?file=934923#file934923line82>
> >
> > I'm wondering why we are removing this one?
>
> Abraham Elmahrek wrote:
> It turns out overriding a method that is annotated with @BeforeSuite is not correct. So I've removed it and am using HadoopMiniClusterRunner.class as the default in TomcatTestCase.
Got it - so even the tests that do not necessary requires the HadoopMiniCluster will run on it. I guess that it's fair even though not absolutely required.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, lines 98-103
> > <https://reviews.apache.org/r/31824/diff/3/?file=934925#file934925line98>
> >
> > If we are defining the method name in the parent class of all tests, let's drop it from the children test classes? I know that several tests are using this functionality.
>
> Abraham Elmahrek wrote:
> Indeed. I've changed the getTestName() methods in the other classes as well. Now, it uses context.getName() everywhere to ensure that when this method is called, there won't be an NPE. Maybe we can move it higher up in the inheritance chain?
Yeah I think so :)
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, line 108
> > <https://reviews.apache.org/r/31824/diff/3/?file=934925#file934925line108>
> >
> > We're changing the default value here, so I'm wondering if it's intentional?
>
> Abraham Elmahrek wrote:
> Completely intentional. ConnectorTestCase was overriding this and using a different default. I've removed the method overriding this one in ConnectorTestCase and changed the default here.
Got it, I'm +1 then.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java, lines 49-177
> > <https://reviews.apache.org/r/31824/diff/3/?file=934927#file934927line49>
> >
> > Seems as a lot of unrelated changes and moves around?
>
> Abraham Elmahrek wrote:
> Actually, "beforeMethod" was completely removed because it wasn't being executed in time any more. The test name will be context.getName() until TomcatTestCase.startServer is ran. I did move getTestName() unintentionally. It took me a while to get any of this working correctly. Let me know if you'd like to move getTestName() back to its original place.
Not necessarily - the change just wasn't obvious to me. My apologies for that.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/#review80876
-----------------------------------------------------------
On April 19, 2015, 9:02 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31824/
> -----------------------------------------------------------
>
> (Updated April 19, 2015, 9:02 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1952
> https://issues.apache.org/jira/browse/SQOOP-1952
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit d489e803cc01127ded5270bbe7cf640e674648aa
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Mar 6 17:15:06 2015 -0800
>
> SQOOP-1952
>
> :100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
> :100644 100644 32afc77... 1b30782... M pom.xml
> :100644 100644 f743d25... d2761f6... M test/pom.xml
> :100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
> :000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
>
>
> Diffs
> -----
>
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java b26ca74
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java 09ddcc7
> pom.xml c608ca7
> test/pom.xml d8fbfa2
> test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java 020fa3f
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
> test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java 9aa69ed
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
> test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java 37a0a7d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java 88db2f2
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java 92a52b8
> test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java 5c90501
> test/src/test/resources/suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31824/diff/
>
>
> Testing
> -------
>
> Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > Hi Abe,
> > thank you very much for working on this one! I have several questions/suggestions inline and a high level questions:
> >
> > It seems that when running integration tests, all that user sees is this output on the console:
> >
> > Running TestSuite
> > Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 905.703 sec
> >
> > I'm wondering if there is a way how to report more granual progress then "working" and "done"? Like "Executed 5 from 26 tests" or something similar? I'm a bit concerned that this is step back in usability as don't have easy way to see what are the tests doing - we would have to wait entire time for them to finish to even noticed that something is failing.
Yeah good point. I can dig a bit to find out what we can do here.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/test/resources/suite.xml, line 21
> > <https://reviews.apache.org/r/31824/diff/3/?file=934932#file934932line21>
> >
> > Do you think that we will add more suits in the future? If so, it would perhaps make sense to rename the file to "IntegrationTests.xml" or something similar right?
Yep. Good point.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/test/resources/suite.xml, lines 25-29
> > <https://reviews.apache.org/r/31824/diff/3/?file=934932#file934932line25>
> >
> > I'm wondering if it would be feasilbe to use org.apache.integration.connector here? I think that it will be error prone to force us to create new entry for single new package that we will create.
Yep. Good point.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java, lines 49-177
> > <https://reviews.apache.org/r/31824/diff/3/?file=934927#file934927line49>
> >
> > Seems as a lot of unrelated changes and moves around?
Actually, "beforeMethod" was completely removed because it wasn't being executed in time any more. The test name will be context.getName() until TomcatTestCase.startServer is ran. I did move getTestName() unintentionally. It took me a while to get any of this working correctly. Let me know if you'd like to move getTestName() back to its original place.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java, line 46
> > <https://reviews.apache.org/r/31824/diff/3/?file=934927#file934927line46>
> >
> > It seems that we're re-classifying this test, so I'm wondering whether this should be done in separate JIRA?
Yep.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, lines 98-103
> > <https://reviews.apache.org/r/31824/diff/3/?file=934925#file934925line98>
> >
> > If we are defining the method name in the parent class of all tests, let's drop it from the children test classes? I know that several tests are using this functionality.
Indeed. I've changed the getTestName() methods in the other classes as well. Now, it uses context.getName() everywhere to ensure that when this method is called, there won't be an NPE. Maybe we can move it higher up in the inheritance chain?
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java, line 108
> > <https://reviews.apache.org/r/31824/diff/3/?file=934925#file934925line108>
> >
> > We're changing the default value here, so I'm wondering if it's intentional?
Completely intentional. ConnectorTestCase was overriding this and using a different default. I've removed the method overriding this one in ConnectorTestCase and changed the default here.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java, lines 82-94
> > <https://reviews.apache.org/r/31824/diff/3/?file=934923#file934923line82>
> >
> > I'm wondering why we are removing this one?
It turns out overriding a method that is annotated with @BeforeSuite is not correct. So I've removed it and am using HadoopMiniClusterRunner.class as the default in TomcatTestCase.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java, line 44
> > <https://reviews.apache.org/r/31824/diff/3/?file=934924#file934924line44>
> >
> > I'm wondering if this change is related to the patch? I think that it's a good idea to make the topic name configurable, but that seems as orthogonal issue and hence should be perhaps solved in separate JIRA?
Actually, I'm not sure. In order to get the kafka tests working I had to play around with a few different mechanisms: topic name and zookeeper configuration. I might be able to remove this given the Zookeeper configuration changes. I'll check.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java, line 27
> > <https://reviews.apache.org/r/31824/diff/3/?file=934922#file934922line27>
> >
> > Is this intentional change?
not at all :)
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/#review80876
-----------------------------------------------------------
On April 19, 2015, 9:02 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31824/
> -----------------------------------------------------------
>
> (Updated April 19, 2015, 9:02 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1952
> https://issues.apache.org/jira/browse/SQOOP-1952
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit d489e803cc01127ded5270bbe7cf640e674648aa
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Mar 6 17:15:06 2015 -0800
>
> SQOOP-1952
>
> :100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
> :100644 100644 32afc77... 1b30782... M pom.xml
> :100644 100644 f743d25... d2761f6... M test/pom.xml
> :100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
> :000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
>
>
> Diffs
> -----
>
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java b26ca74
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java 09ddcc7
> pom.xml c608ca7
> test/pom.xml d8fbfa2
> test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java 020fa3f
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
> test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java 9aa69ed
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
> test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java 37a0a7d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java 88db2f2
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java 92a52b8
> test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java 5c90501
> test/src/test/resources/suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31824/diff/
>
>
> Testing
> -------
>
> Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On April 21, 2015, 2:13 a.m., Jarek Cecho wrote:
> > Hi Abe,
> > thank you very much for working on this one! I have several questions/suggestions inline and a high level questions:
> >
> > It seems that when running integration tests, all that user sees is this output on the console:
> >
> > Running TestSuite
> > Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 905.703 sec
> >
> > I'm wondering if there is a way how to report more granual progress then "working" and "done"? Like "Executed 5 from 26 tests" or something similar? I'm a bit concerned that this is step back in usability as don't have easy way to see what are the tests doing - we would have to wait entire time for them to finish to even noticed that something is failing.
>
> Abraham Elmahrek wrote:
> Yeah good point. I can dig a bit to find out what we can do here.
>
> Jarek Cecho wrote:
> I think that it's not "absolute must have" if this will help us to speed up the tests, but I think that it would be quite desirable :)
I don't think there's a built in way to do this. I can create a follow up Jira to add a custom listener for this.
It is a stepback in usability in some sense. This patch should improve test speeds at the very least.
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/#review80876
-----------------------------------------------------------
On April 19, 2015, 9:02 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31824/
> -----------------------------------------------------------
>
> (Updated April 19, 2015, 9:02 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1952
> https://issues.apache.org/jira/browse/SQOOP-1952
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit d489e803cc01127ded5270bbe7cf640e674648aa
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Mar 6 17:15:06 2015 -0800
>
> SQOOP-1952
>
> :100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
> :100644 100644 32afc77... 1b30782... M pom.xml
> :100644 100644 f743d25... d2761f6... M test/pom.xml
> :100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
> :000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
>
>
> Diffs
> -----
>
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java b26ca74
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java 09ddcc7
> pom.xml c608ca7
> test/pom.xml d8fbfa2
> test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java 020fa3f
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
> test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java 9aa69ed
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
> test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java 37a0a7d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java 88db2f2
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java 92a52b8
> test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java 5c90501
> test/src/test/resources/suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31824/diff/
>
>
> Testing
> -------
>
> Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/#review80876
-----------------------------------------------------------
Hi Abe,
thank you very much for working on this one! I have several questions/suggestions inline and a high level questions:
It seems that when running integration tests, all that user sees is this output on the console:
Running TestSuite
Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 905.703 sec
I'm wondering if there is a way how to report more granual progress then "working" and "done"? Like "Executed 5 from 26 tests" or something similar? I'm a bit concerned that this is step back in usability as don't have easy way to see what are the tests doing - we would have to wait entire time for them to finish to even noticed that something is failing.
common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java
<https://reviews.apache.org/r/31824/#comment131017>
This seems unrelated change to this patch?
common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java
<https://reviews.apache.org/r/31824/#comment131018>
I'm wondering if I'm missing something or whether this just moves the code around? If so, is it really required?
test/pom.xml
<https://reviews.apache.org/r/31824/#comment131019>
Would it make sense to drop the excludes when we're not using them anymore?
test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java
<https://reviews.apache.org/r/31824/#comment131020>
Is this intentional change?
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
<https://reviews.apache.org/r/31824/#comment131022>
I'm wondering why we are removing this one?
test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java
<https://reviews.apache.org/r/31824/#comment131024>
I'm wondering if this change is related to the patch? I think that it's a good idea to make the topic name configurable, but that seems as orthogonal issue and hence should be perhaps solved in separate JIRA?
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
<https://reviews.apache.org/r/31824/#comment131025>
If we are defining the method name in the parent class of all tests, let's drop it from the children test classes? I know that several tests are using this functionality.
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
<https://reviews.apache.org/r/31824/#comment131026>
We're changing the default value here, so I'm wondering if it's intentional?
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java
<https://reviews.apache.org/r/31824/#comment131028>
I'm wondering what is the reasoning to drop the directory containing all the tests? That will make it quite hard to follow as target/ will be full of "random" directories right? Isn't it better to put together all the tests into the same subdirectory like target/cargo-test-output/*?
test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java
<https://reviews.apache.org/r/31824/#comment131029>
I'm not sure that I'm following up this change correctly, are we simply removing unused variable?
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java
<https://reviews.apache.org/r/31824/#comment131031>
It seems that we're re-classifying this test, so I'm wondering whether this should be done in separate JIRA?
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java
<https://reviews.apache.org/r/31824/#comment131030>
Seems as a lot of unrelated changes and moves around?
test/src/test/resources/suite.xml
<https://reviews.apache.org/r/31824/#comment131035>
Do you think that we will add more suits in the future? If so, it would perhaps make sense to rename the file to "IntegrationTests.xml" or something similar right?
test/src/test/resources/suite.xml
<https://reviews.apache.org/r/31824/#comment131032>
I'm wondering if it would be feasilbe to use org.apache.integration.connector here? I think that it will be error prone to force us to create new entry for single new package that we will create.
Jarcec
- Jarek Cecho
On April 19, 2015, 9:02 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31824/
> -----------------------------------------------------------
>
> (Updated April 19, 2015, 9:02 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1952
> https://issues.apache.org/jira/browse/SQOOP-1952
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit d489e803cc01127ded5270bbe7cf640e674648aa
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Mar 6 17:15:06 2015 -0800
>
> SQOOP-1952
>
> :100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
> :100644 100644 32afc77... 1b30782... M pom.xml
> :100644 100644 f743d25... d2761f6... M test/pom.xml
> :100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
> :000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
>
>
> Diffs
> -----
>
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java b26ca74
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java 09ddcc7
> pom.xml c608ca7
> test/pom.xml d8fbfa2
> test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java 020fa3f
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
> test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java 9aa69ed
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
> test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java 37a0a7d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java 88db2f2
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java 92a52b8
> test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java 5c90501
> test/src/test/resources/suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31824/diff/
>
>
> Testing
> -------
>
> Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/#review81417
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On April 19, 2015, 9:02 a.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31824/
> -----------------------------------------------------------
>
> (Updated April 19, 2015, 9:02 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1952
> https://issues.apache.org/jira/browse/SQOOP-1952
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> commit d489e803cc01127ded5270bbe7cf640e674648aa
> Author: Abraham Elmahrek <ab...@apache.org>
> Date: Fri Mar 6 17:15:06 2015 -0800
>
> SQOOP-1952
>
> :100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
> :100644 100644 32afc77... 1b30782... M pom.xml
> :100644 100644 f743d25... d2761f6... M test/pom.xml
> :100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
> :000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
>
>
> Diffs
> -----
>
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java b26ca74
> common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java 09ddcc7
> pom.xml c608ca7
> test/pom.xml d8fbfa2
> test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java 020fa3f
> test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
> test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java 9aa69ed
> test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
> test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java 37a0a7d
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
> test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java 88db2f2
> test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java 92a52b8
> test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java 5c90501
> test/src/test/resources/suite.xml PRE-CREATION
>
> Diff: https://reviews.apache.org/r/31824/diff/
>
>
> Testing
> -------
>
> Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 31824: Sqoop2: Load miniclusters at beginning of
integration tests
Posted by Abraham Elmahrek <ab...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31824/
-----------------------------------------------------------
(Updated April 19, 2015, 9:02 a.m.)
Review request for Sqoop.
Bugs: SQOOP-1952
https://issues.apache.org/jira/browse/SQOOP-1952
Repository: sqoop-sqoop2
Description
-------
commit d489e803cc01127ded5270bbe7cf640e674648aa
Author: Abraham Elmahrek <ab...@apache.org>
Date: Fri Mar 6 17:15:06 2015 -0800
SQOOP-1952
:100755 100755 b1acdc4... 4a5ce80... M dev-support/test-patch.py
:100644 100644 32afc77... 1b30782... M pom.xml
:100644 100644 f743d25... d2761f6... M test/pom.xml
:100644 100644 ce6af6e... 90afef1... M test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java
:000000 100644 0000000... 2febf5e... A test/src/test/resources/suite.xml
Diffs (updated)
-----
common-test/src/main/java/org/apache/sqoop/common/test/kafka/KafkaRunnerFactory.java b26ca74
common-test/src/main/java/org/apache/sqoop/common/test/kafka/TestUtil.java 09ddcc7
pom.xml c608ca7
test/pom.xml d8fbfa2
test/src/main/java/org/apache/sqoop/test/hadoop/HadoopRunnerFactory.java 020fa3f
test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java c8fca68
test/src/main/java/org/apache/sqoop/test/testcases/KafkaConnectorTestCase.java 9aa69ed
test/src/main/java/org/apache/sqoop/test/testcases/TomcatTestCase.java 9416473
test/src/test/java/org/apache/sqoop/integration/connector/hive/FromRDBMSToKiteHiveTest.java 37a0a7d
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/AllTypesTest.java 4e46637
test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/IncrementalReadTest.java 5bde35c
test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromHDFSToKafkaTest.java 88db2f2
test/src/test/java/org/apache/sqoop/integration/connector/kafka/FromRDBMSToKafkaTest.java 92a52b8
test/src/test/java/org/apache/sqoop/integration/connector/kite/FromRDBMSToKiteTest.java 5c90501
test/src/test/resources/suite.xml PRE-CREATION
Diff: https://reviews.apache.org/r/31824/diff/
Testing
-------
Tested locally. Also, tested against SQOOP-1781 jira and posted results there. They seem mixed largely because it's difficult to provide changes to testing and changes to dev support together.
Thanks,
Abraham Elmahrek