sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Attila Szabo <mau...@apache.org>
Subject Re: Review Request 58233: SQOOP-3167: Evalutation and automation of SQLServer Manual tests
Date Fri, 07 Apr 2017 17:25:21 GMT

This is an automatically generated e-mail. To reply, visit:

Hi Bogi, Liz,

Most probably I did not phrase my thoughts precise enough in the previous round, so please
let me express myself in a bit more direct and clear way:
My problem is not with the size of the patch file (although IMHO ~1000+ changed lines is quite
a few), but with the fact that this change tries to achieve multiple things, which I think
would make more sense as part of multiple issues/commits. Here are the things  I've identified
as the effects/consequences of this changeset:
- It delivers improvements/fixes around the way connections are handled in the MSSqlServer
related tests, which is a great thing, makes the usage much more flexible.
-- Although around the deletes (were those codes really dead?)/fixes I'm not sure if those
things would be in sync with the design goals of the SqlServer connector, but I'm not used
to consider myself as a MSSqlServer expert.
- Pushes the SqlServer related manual tests into the third party ant test cycle. IMHO this
is not the best decision in the current test + CI architecture, as from that moment this patch
would have been committed, it would force every contributor to have a working MSSqlServer
instance (on the top of the currently existing ones including MySQL, PostgreSQL, Oracle, Cubrid,
etc.) on their dev infrastructure, or facing with the fact that some of the third party tests
will fail continuously on their side (which does not sound like a best practice). Most probably
on your side it didn't appear as a problem as you do have standalone instances for yourself,
but we cannot depend on that this is true in case of every contributor. Although the test
files themselves contains some very basic instructions how to make the tests work, but still
in the current version it would need manual interactions+installs, thus won't work out of
the box (needless to say that the instructions are very much not deta
 iled enough to someone who is not expert in installing MSSqlServer).
-- As a subpart of this section, I have to highlight that this way of changes alternates the
intention of the original devs (and I'm pretty sure they had good reason why these tests were
not automated but manual).
-- Introducing just another external dependency won't help to onboard new contributors (just
another new -D param for a process which is not quite good documented already)
-- It also won't make the CI integration easier in the future
- The title+description JIRA is missleading as it does not solve the true automation (neither
on CI level, nor on the side of the devs).

So if you trust in my experience and wisdom as a committer you would consider splitting up
this changeset into 2-3 parts, and only after pushing the tests into the 3rd party cycle,
when the SqlServer deployment is solved out of the box (e.g. with docker or global apache
installation or ansible or so).

Please also consider fixing that part what I've identified as an issue

My 2 cents,

Lines 224-242 (original), 249-299 (patched)

    Could you please give me some explanation around these test cases?
    For me it looks strange intentionally, that we're trying to leverage from some base/super
test class, but nullifying some of it's test cases with an empty method body.
    If these things are not neccessary I would suggest deleting them, as the current solution
is very much misleading, b/c these cases won't look as 'invalid' as you marked them, but very
much passed green test cases, giving the false intention these cases are very much supported
in the delimited case as well.
    If you need a common ancestor I would advise using 'extract superclass' refactoring.

- Attila Szabo

On April 6, 2017, 5:27 p.m., Boglarka Egyed wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58233/
> -----------------------------------------------------------
> (Updated April 6, 2017, 5:27 p.m.)
> Review request for Sqoop, Anna Szonyi, Szabolcs Vasas, and Liz Szilagyi.
> Bugs: SQOOP-3167
>     https://issues.apache.org/jira/browse/SQOOP-3167
> Repository: sqoop-trunk
> Description
> -------
> Automated SQLServer Manual tests including test case correction and minor rework too:
> - modified connection string setup to make them able to run automatically
> - fixed failing test cases
> - excluded invalid test cases
> - added database cleanup logic in tearDown part
> - updated java docs
> - removed unused imports
> - changing names to add them to the 3rd party test suite
> Note: A more extensive refactor of the test classes in org.apache.sqoop.manager.sqlserver
could be made, it will be addressed in another JIRA as that is a different scope.
> Diffs
> -----
>   build.xml 73db28b272c50b4f76fef8421e6b9dfe5fed40f4 
>   src/test/com/cloudera/sqoop/hive/TestHiveImport.java 33e0cc41f6f379bac2085431e0f1adc60bce6bce

>   src/test/com/cloudera/sqoop/manager/SQLServerManagerExportManualTest.java 9a92479245fa35c210d8e49f847292ee53d6f9b1

>   src/test/com/cloudera/sqoop/manager/SQLServerManagerImportManualTest.java 1f69725da8408853ac55b1f316ce1b9ef015e674

>   src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java 851bf49614e829d07de252b83f4ad550d0cb043b

>   src/test/org/apache/sqoop/manager/sqlserver/ManagerCompatExport.java 8c5176ad61aae61b96c7458d3b4b83dc11960268

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportDelimitedFileManualTest.java
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportSequenceFileManualTest.java
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportDelimitedFileManualTest.java
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportSequenceFileManualTest.java
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportManualTest.java 1999272181421a539318ed195ea4257f52b2ed08

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerManualTest.java 1178e3c79de4d0b5c7a96c6ad7eb316ed15e47c4

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiColsManualTest.java 6a8ab51967237f471044b868615fdb3e057b1d92

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerMultiMapsManualTest.java c9a5b5ef596cfc1b28948c2a071935dfb9500cde

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerParseMethodsManualTest.java cd05aecf1ae5bd79fb485325d58b33a73e9df290

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerQueryManualTest.java 0057ac9df562c8e92cf7b9014c5e4239886a8104

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerSplitByManualTest.java f85245ab8cdd66da983ac9017d356f251f22e7db

>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerWhereManualTest.java 10ae03b324b15f5ea0cc3cbbc04d3a5041233dd9

> Diff: https://reviews.apache.org/r/58233/diff/2/
> Testing
> -------
> ant clean test, ant test
> ant clean test -Dthirdparty=true -Dsqoop.thirdparty.lib.dir=3rdpartylib -Dsqoop.test.sqlserver.connectstring.host_url=sqlserver
-Dsqoop.test.sqlserver.database=databasename -Dms.sqlserver.username=username -Dms.sqlserver.password=password
> ant clean test -Dthirdparty=true -Dsqoop.thirdparty.lib.dir=3rdpartylib -Dsqoop.test.mysql.connectstring.host_url=mysql
-Dsqoop.test.mysql.databasename=databasename -Dsqoop.test.mysql.password=password -Dsqoop.test.mysql.username=username
-Dsqoop.test.oracle.connectstring=oracle -Dsqoop.test.postgresql.connectstring.host_url=postgre
-Dsqoop.test.cubrid.connectstring.host_url=cubrid -Dsqoop.test.cubrid.connectstring.username=username
-Dsqoop.test.cubrid.connectstring.database=database -Dsqoop.test.cubrid.connectstring.password=password
-Dmapred.child.java.opts="-Djava.security.egd=file:/dev/../dev/urandom" -Dtest.timeout=1000000
-Dsqoop.test.sqlserver.connectstring.host_url=sqlserver -Dsqoop.test.sqlserver.database=database
-Dms.sqlserver.username=username -Dms.sqlserver.password=password
> Thanks,
> Boglarka Egyed

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