sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Teoh <chris.t...@gmail.com>
Subject Re: Review Request 67429: SQOOP-3331: Add mainframe integration test for GDG
Date Fri, 15 Jun 2018 00:19:10 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message