> On April 7, 2017, 5:25 p.m., Attila Szabo wrote: > > 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 detailed 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, > > Attila > > Attila Szabo wrote: > I suggest discussing this on dev@sqoop list. > If the PMC agrees on adding SqlServer integration tests to the current > test suite, I'd be happy to merge your patch.
Thanks Attila for your verbose answer and suggestions, I discard this review and cancel the patch. I have created another JIRA with a reduced scope, see SQOOP-3169 Regards, Bogi - Boglarka ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58233/#review171363 ----------------------------------------------------------- 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 > 099d7344beb428c58b32d926af5ea079211da490 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeExportSequenceFileManualTest.java > 21676f02510693dcdd856a1d9dfba7d05eace023 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportDelimitedFileManualTest.java > 519fb525bdbb167520368d404667036669925041 > > src/test/org/apache/sqoop/manager/sqlserver/SQLServerDatatypeImportSequenceFileManualTest.java > a0dad8a60b99d522ad3691e15b8b16c56e4b5858 > > 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 -Dtestcase=SQLServer* > > 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 > >
