Just a small comment. Let's make sure that going forward we discuss ideas based on their merit and not justify them with an appeal to authority. There is no need to "trust in anyone's experience and wisdom" when a meritocracy is operating properly.
Thanks, Abe On Mon, Apr 10, 2017, at 05:51, Boglarka Egyed wrote: > > > > 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 > > > > >
