> On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > Hi Chris! > > > > Thank you for your patch this is a great addition to our mainframe > > connector testing! > > Could we use this image for the other mainframe tests as well? > > > > When I tried to run it on my machine I got an exception first the reason > > was that org.apache.sqoop.manager.MainframeManager#options had to be > > deleted because it is already added to ConnManager, can you please include > > this change in your patch as well? > > See my other comments inline.
Yes it can be used for Sequential datasets as well. I have not yet put in any testing for Partitioned Datasets as I'm not using those at the moment. Yes I can add the patch. > On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > build.xml > > Lines 887 (patched) > > <https://reviews.apache.org/r/67429/diff/3/?file=2035226#file2035226line887> > > > > Do we need to be able to set gdg, gdg.filename and gdg.md5 from Ant? > > For host, port, username and password it makes sense because in theory > > they could be different in some test environments but gdg, gdg.filename and > > gdg.md5 seems to be a test data which could be defined as a static field in > > your test only. I put them in for flexibility. I have already set the default values in the file as well so if they don't get overridden, we shouldn't have a problem. Let me know if you want this changed. ``` <property name="sqoop.test.mainframe.ftp.host" value="localhost" /> <property name="sqoop.test.mainframe.ftp.port" value="2121" /> <property name="sqoop.test.mainframe.ftp.username" value="test" /> <property name="sqoop.test.mainframe.ftp.password" value="test" /> <property name="sqoop.test.mainframe.ftp.dataset.gdg" value="TSODIQ1.GDGTEXT" /> <property name="sqoop.test.mainframe.ftp.dataset.gdg.filename" value="G0001V43" /> <property name="sqoop.test.mainframe.ftp.dataset.gdg.md5" value="f0d0d171fdb8a03dbc1266ed179d7093" /> ``` > On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > src/scripts/thirdpartytest/docker-compose/sqoop-thirdpartytest-db-services.yml > > Lines 115 (patched) > > <https://reviews.apache.org/r/67429/diff/3/?file=2035227#file2035227line115> > > > > Did you create this image using a dockerfile? If yes, is it possible to > > attach to include it in your patch? Thanks for your review. I have the Dockerfile and accompanying contents at https://github.com/christeoh/zos-ftpmock/ Let me know if I need to do anything else to reference this. I will also mention it in the JIRA. > On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > > Lines 42 (patched) > > <https://reviews.apache.org/r/67429/diff/3/?file=2035228#file2035228line42> > > > > There are unfortunately too many StringUtils classes on the classpath, > > can you please use org.apache.commons.lang3.StringUtils? Thanks. I have updated this. > On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > > Lines 112 (patched) > > <https://reviews.apache.org/r/67429/diff/3/?file=2035228#file2035228line112> > > > > Do we need this method here? It initializes a MainframeManager but it > > is not used later and I can see that similar parameters are configured in > > getArgv method. Yes. The base class was throwing errors saying it did not recognise the mainframe parameters. > On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > > Lines 163 (patched) > > <https://reviews.apache.org/r/67429/diff/3/?file=2035228#file2035228line163> > > > > Do we need to redefine runImport here or can we just use > > org.apache.sqoop.testutil.ImportJobTestCase#runImport(org.apache.sqoop.tool.SqoopTool, > > java.lang.String[])? Yes, I did not want to use the base function as it called getSqoopOptions(conf) and creates a new SqoopOptions instance. I put in some configuration values in SqoopOptions instance in my setup() which are needed for the test. > On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > > Lines 185 (patched) > > <https://reviews.apache.org/r/67429/diff/3/?file=2035228#file2035228line185> > > > > Can we remove this comment? Done. > On June 14, 2018, 10:35 p.m., Szabolcs Vasas wrote: > > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > > Lines 216 (patched) > > <https://reviews.apache.org/r/67429/diff/3/?file=2035228#file2035228line216> > > > > You can remove this method since it is empty. Done. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67429/#review204767 ----------------------------------------------------------- On June 15, 2018, 10:18 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67429/ > ----------------------------------------------------------- > > (Updated June 15, 2018, 10:18 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-trunk > > > Description > ------- > > Added simple test for GDG dataset. Fixed bug triggered by recent Parquet > patch. > > > Diffs > ----- > > build.xml a85705fa > src/java/org/apache/sqoop/manager/MainframeManager.java 4e8be155 > > src/scripts/thirdpartytest/docker-compose/sqoop-thirdpartytest-db-services.yml > 2f4a07f6 > src/test/org/apache/sqoop/manager/mainframe/MainframeManagerImportTest.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/67429/diff/4/ > > > Testing > ------- > > Running the integration and unit test on a patched version of trunk. > > > Thanks, > > Chris Teoh > >
