Re: Review Request 62523: SQOOP-3237: Mainframe FTP transfer option to insert custom FTP commands prior to transfer

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62523/#review211186
---


Fix it, then Ship it!




Hi Chris,

I also had a look at this one. Nice, clean patch, good job!

I just had a few comments on formatting, no biggies. We should probably 
automate this formatting stuff in the future somehow. Also the removal of 
unused imports. 

Best Regards,
Fero


src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java
Lines 78 (patched)


You could also use isNotBlank next time :)

That's slightly easier on my eyes.



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 281 (patched)


Missing space. Should read like this:
LOG.info("Issuing command: " + ftpCommand);

I'm not sure if we have a formatting guidline like this, but we probably 
should if we don't.



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 285 (patched)


Also missing space around the + operators.

LOG.info("ReplyCode: " + res + " ReplyString: " + result);



src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
Lines 59 (patched)


This new line is unnecessary here.



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 21-22 (patched)


Unused import



src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java
Lines 34 (patched)


Unused import.


- Fero Szabo


On Dec. 10, 2018, 5:45 a.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62523/
> ---
> 
> (Updated Dec. 10, 2018, 5:45 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3237
> https://issues.apache.org/jira/browse/SQOOP-3237
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Added --ftpcmds command to allow comma separated list of FTP commands to send.
> 
> 
> Diffs
> -
> 
>   src/docs/user/import-mainframe.txt 3ecfb7e4 
>   src/java/org/apache/sqoop/SqoopOptions.java f06872f9 
>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
> 9842daa6 
>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java 
> 90dc2ddd 
>   src/java/org/apache/sqoop/tool/MainframeImportTool.java fbc8c3db 
>   src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java e7c48a6b 
>   
> src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
>  502e6333 
>   src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 00e57bd0 
>   src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java fc6e56d6 
> 
> 
> Diff: https://reviews.apache.org/r/62523/diff/11/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> 
> File Attachments
> 
> 
> SQOOP-3237-1.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/09/26/56041556-e355-4372-83ab-1bcc01680201__SQOOP-3237-1.patch
> 
> 
> Thanks,
> 
> Chris Teoh
> 
>

Re: Review Request 69434: SQOOP-3410: Test S3 import with fs.s3a.security.credential.provider.path

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69434/#review210832
---


Ship it!




Lgtm

- Fero Szabo


On Nov. 22, 2018, 4:06 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69434/
> ---
> 
> (Updated Nov. 22, 2018, 4:06 p.m.)
> 
> 
> Review request for Sqoop, Fero Szabo and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3410
> https://issues.apache.org/jira/browse/SQOOP-3410
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Based on 
> https://hadoop.apache.org/docs/current/hadoop-aws/tools/hadoop-aws/index.html#Configure_the_hadoop.security.credential.provider.path_property
>  property fs.s3a.security.credential.provider.path can also be used for 
> passing the location of the credential store. This should be also tested and 
> documented.
> 
> 
> Diffs
> -
> 
>   src/docs/user/s3.txt 6ff828c497e0711a2394f768ed5d61ecaf9ec273 
>   src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java 
> 4e79f0ae252969c4a426d1ff69072695eb37b7a6 
>   src/test/org/apache/sqoop/credentials/TestPassingSecurePassword.java 
> dca3195b8051048c5c7c2fb3bf30774e9d19eda8 
>   src/test/org/apache/sqoop/s3/TestS3ImportWithHadoopCredProvider.java 
> e1d7cbda2c65aa59a89715adff52b85fb3730477 
> 
> 
> Diff: https://reviews.apache.org/r/69434/diff/1/
> 
> 
> Testing
> ---
> 
> ant clean test
> ./gradlew -Ds3.bucket.url= 
> -Ds3.generator.command= s3Test --tests 
> TestS3ImportWithHadoopCredProvider
> 
> ant clean docs
> ./gradlew docs
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 69433: Setting up Travis CI using Gradle test categories

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69433/#review210825
---


Ship it!




Hi Szabi,

not an issue, just an idea.

In any case, tests are passing, and it looks good.


COMPILING.txt
Lines 413 (patched)


I think it would make sense to try to pick up the oracle driver from 
/var/lib/sqoop. What do you think?


- Fero Szabo


On Nov. 23, 2018, 10:33 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69433/
> ---
> 
> (Updated Nov. 23, 2018, 10:33 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3289
> https://issues.apache.org/jira/browse/SQOOP-3289
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> The patch includes the following changes:
> - Changed the default DB connection parameters to Docker image defaults so 
> the test tasks can be started without specifying connection parameters
> - Connection parameter settings duplications are removed
> - Most of the JDBC drivers are downloaded from Maven repositories the only 
> exception is Oracle. Contributors have to upload ojdbc6.jar to a public drive 
> and make it available to the CI job by setting the ORACLE_DRIVER_URL in Travis
> - Introduced separate test tasks for each databases
> - An Oracle Express Edition Docker image is added to 
> sqoop-thirdpartytest-db-services.yml so Oracle tests which does not require 
> Oracle EE features can be executed much easier
> - The ports for MySQL and PostgreSQL Docker containers are changed because 
> the default ones were used in the Travis VM already.
> - Introduced OracleEe test category for tests requiring Oracle EE database. 
> These tests won't be executed on Travis. The good news is that only a few 
> tests require Oracle EE
> 
> Documentation is still coming feel free to provide a feedback!
> 
> 
> Diffs
> -
> 
>   .travis.yml PRE-CREATION 
>   COMPILING.txt b399ba825 
>   build.gradle efe980d67 
>   build.xml a0e25191e 
>   gradle.properties 722bc8bb2 
>   src/scripts/thirdpartytest/docker-compose/oraclescripts/ee-healthcheck.sh 
> PRE-CREATION 
>   src/scripts/thirdpartytest/docker-compose/oraclescripts/healthcheck.sh 
> fb7800efe 
>   
> src/scripts/thirdpartytest/docker-compose/sqoop-thirdpartytest-db-services.yml
>  b4cf48863 
>   src/test/org/apache/sqoop/manager/cubrid/CubridTestUtils.java 4fd522bae 
>   
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManualTest.java
>  ed949b98f 
>   src/test/org/apache/sqoop/manager/db2/DB2ManagerImportManualTest.java 
> 32dfc5eb2 
>   src/test/org/apache/sqoop/manager/db2/DB2TestUtils.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> 494c75b08 
>   src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java be205c877 
>   src/test/org/apache/sqoop/manager/oracle/ExportTest.java a60168719 
>   src/test/org/apache/sqoop/manager/oracle/ImportTest.java 5db9fe34e 
>   src/test/org/apache/sqoop/manager/oracle/OraOopTestCase.java 1598813d8 
>   src/test/org/apache/sqoop/manager/oracle/OraOopTypesTest.java 1f67c4697 
>   src/test/org/apache/sqoop/manager/oracle/OracleConnectionFactoryTest.java 
> 34e182f4c 
>   src/test/org/apache/sqoop/manager/oracle/TimestampDataTest.java be086c5c2 
>   src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java 14b57f91a 
>   
> src/test/org/apache/sqoop/manager/postgresql/DirectPostgreSQLExportManualTest.java
>  7dd6efcf9 
>   
> src/test/org/apache/sqoop/manager/postgresql/PGBulkloadManagerManualTest.java 
> 1fe264456 
>   src/test/org/apache/sqoop/manager/postgresql/PostgresqlExportTest.java 
> eb798fa99 
>   
> src/test/org/apache/sqoop/manager/postgresql/PostgresqlExternalTableImportTest.java
>  8c3d2fd90 
>   src/test/org/apache/sqoop/manager/postgresql/PostgresqlTestUtil.java 
> e9705e5da 
>   src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java bd12c5566 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerExportTest.java 
> ab1e8ff2d 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
> 3c5bb327e 
>   src/test/org/apache/sqoop/metastore/db2/DB2JobToolTest.java 81ef5fce6 
>   
> src/test/org/apache/sqoop/metastore/db2/DB2MetaConnectIncrementalImportTest.java
>  5403908e2 
>   src/test/org/apache/sqoop/metastore/db2/DB2SavedJobsTest.java b41eda110 
>   src/test/org/apache/sqoop/metastore/postgres/PostgresJobToolTest.java 
> 59ea151a5 
>   
> src/test/org/apache/sqoop/metastore/postgres/PostgresMetaConnectIncrementalImportTest.java
>  afc6bd232 
>   src/test/org/apache/sqoop/metastore/postgres/PostgresSavedJobsTest.java 
> 9f9e865b9 
>   

Re: Review Request 69433: Setting up Travis CI using Gradle test categories

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69433/#review210809
---



Hi Szabi,

The whole change looks good to me, haven't spotted any mistakes, though still 
need to run tests.

Just some questions to clarify my understanding of the change:

I see the build.xml contains the default values for the connection strings. 
1. How do these get picked up by the docker images?
I'm guessing that I can see portforwarding in the yml that's the input for 
docker compose, this would answer it.

2. And how does gradle pick 'em up?
I think this is why you've modified the util classes throughout Sqoop. Is that 
correct?

So, what are the modifications in the build.xml needed for?


build.xml
Line 193 (original), 197 (patched)


I guess localhost could have stayed (just the port had to be added), or was 
there a problem with it?



src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java
Line 56 (original), 56 (patched)


uppercase / lowercase typo :)

mysqlDbNAme > mysqlDbName


- Fero Szabo


On Nov. 22, 2018, 3:59 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69433/
> ---
> 
> (Updated Nov. 22, 2018, 3:59 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3289
> https://issues.apache.org/jira/browse/SQOOP-3289
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> The patch includes the following changes:
> - Changed the default DB connection parameters to Docker image defaults so 
> the test tasks can be started without specifying connection parameters
> - Connection parameter settings duplications are removed
> - Most of the JDBC drivers are downloaded from Maven repositories the only 
> exception is Oracle. Contributors have to upload ojdbc6.jar to a public drive 
> and make it available to the CI job by setting the ORACLE_DRIVER_URL in Travis
> - Introduced separate test tasks for each databases
> - An Oracle Express Edition Docker image is added to 
> sqoop-thirdpartytest-db-services.yml so Oracle tests which does not require 
> Oracle EE features can be executed much easier
> - The ports for MySQL and PostgreSQL Docker containers are changed because 
> the default ones were used in the Travis VM already.
> - Introduced OracleEe test category for tests requiring Oracle EE database. 
> These tests won't be executed on Travis. The good news is that only a few 
> tests require Oracle EE
> 
> Documentation is still coming feel free to provide a feedback!
> 
> 
> Diffs
> -
> 
>   .travis.yml PRE-CREATION 
>   build.gradle efe980d67 
>   build.xml a0e25191e 
>   gradle.properties 722bc8bb2 
>   src/scripts/thirdpartytest/docker-compose/oraclescripts/ee-healthcheck.sh 
> PRE-CREATION 
>   src/scripts/thirdpartytest/docker-compose/oraclescripts/healthcheck.sh 
> fb7800efe 
>   
> src/scripts/thirdpartytest/docker-compose/sqoop-thirdpartytest-db-services.yml
>  b4cf48863 
>   src/test/org/apache/sqoop/manager/cubrid/CubridTestUtils.java 4fd522bae 
>   
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManualTest.java
>  ed949b98f 
>   src/test/org/apache/sqoop/manager/db2/DB2ManagerImportManualTest.java 
> 32dfc5eb2 
>   src/test/org/apache/sqoop/manager/db2/DB2TestUtils.java PRE-CREATION 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> 494c75b08 
>   src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java be205c877 
>   src/test/org/apache/sqoop/manager/oracle/ExportTest.java a60168719 
>   src/test/org/apache/sqoop/manager/oracle/ImportTest.java 5db9fe34e 
>   src/test/org/apache/sqoop/manager/oracle/OraOopTestCase.java 1598813d8 
>   src/test/org/apache/sqoop/manager/oracle/OraOopTypesTest.java 1f67c4697 
>   src/test/org/apache/sqoop/manager/oracle/OracleConnectionFactoryTest.java 
> 34e182f4c 
>   src/test/org/apache/sqoop/manager/oracle/TimestampDataTest.java be086c5c2 
>   src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java 14b57f91a 
>   
> src/test/org/apache/sqoop/manager/postgresql/DirectPostgreSQLExportManualTest.java
>  7dd6efcf9 
>   
> src/test/org/apache/sqoop/manager/postgresql/PGBulkloadManagerManualTest.java 
> 1fe264456 
>   src/test/org/apache/sqoop/manager/postgresql/PostgresqlExportTest.java 
> eb798fa99 
>   
> src/test/org/apache/sqoop/manager/postgresql/PostgresqlExternalTableImportTest.java
>  8c3d2fd90 
>   src/test/org/apache/sqoop/manager/postgresql/PostgresqlTestUtil.java 
> e9705e5da 
>   src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java bd12c5566 
>   

Re: Review Request 69430: SQOOP-3409: Fix temporary rootdir clean up in Sqoop-S3 tests

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69430/#review210803
---


Ship it!




Ship It!

- Fero Szabo


On Nov. 22, 2018, 1:37 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69430/
> ---
> 
> (Updated Nov. 22, 2018, 1:37 p.m.)
> 
> 
> Review request for Sqoop, Fero Szabo and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3409
> https://issues.apache.org/jira/browse/SQOOP-3409
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Temporary root directory clean up doesn't work as expected, many generated 
> temprootdirs are being kept in the used bucket after test runs. This was 
> caused as the target directory cleanup and name reset happened before the 
> temprootdir cleanup however the temprootdir name depends on the target dir 
> name in the tests.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/testutil/S3TestUtils.java 
> 2fc606115196a7a2b6088be104e2a421888f8798 
> 
> 
> Diff: https://reviews.apache.org/r/69430/diff/1/
> 
> 
> Testing
> ---
> 
> ./gradlew s3Test -Ds3.bucket.url= 
> -Ds3.generator.command=, all the used 
> temprootdirs have been cleaned up
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 69429: Introduce a Gradle build parameter to set the default forkEvery value for the tests

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69429/#review210799
---


Ship it!




Ship It!

- Fero Szabo


On Nov. 22, 2018, 1:11 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69429/
> ---
> 
> (Updated Nov. 22, 2018, 1:11 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3408
> https://issues.apache.org/jira/browse/SQOOP-3408
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Documented forkEvery.default in COMPILING.txt.
> Needed to move the definition of the kerberizedTest task below 
> tasks.withType(Test) block to preserve forkEvery 1 setting.
> 
> 
> Diffs
> -
> 
>   COMPILING.txt 0383707f689102a3a543d94646cfaaf21710 
>   build.gradle 954935daeaaaf45e1b2fd83f74e11f5ed2d58377 
> 
> 
> Diff: https://reviews.apache.org/r/69429/diff/1/
> 
> 
> Testing
> ---
> 
> ./gradlew test : runs the test task with forkEvery=0
> ./gradlew -DforkEvery.default=5 test : runs the test taks with forkEvery=5
> 
> ./gradlew kerberizedTest : runs the kerberizedTest task with forkEvery=1
> ./gradlew -DforkEvery.default=5 kerberizedTest : runs the kerberizedTest task 
> with forkEvery=1, so the forkEvery.default parameter does not affect 
> kerberizedTest
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

> On Nov. 22, 2018, 8:29 a.m., Boglarka Egyed wrote:
> > src/test/org/apache/sqoop/importjob/configuration/MysqlImportJobTestConfiguration.java
> > Lines 24 (patched)
> > 
> >
> > Renamed files are shown as new files now which compromises the diff. 
> > Could you please take a look and regenerate the diff?
> 
> Boglarka Egyed wrote:
> Sorry, I wanted to say that it corrupts the diff.

Fixed with black-belt git magic, with the help of Szabi. ;)


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/#review210785
---


On Nov. 22, 2018, 1:39 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69407/
> ---
> 
> (Updated Nov. 22, 2018, 1:39 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3405
> https://issues.apache.org/jira/browse/SQOOP-3405
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Breaking up the parameterized test classes into a per database basis. 
> Provides better readability, needed for proper test categorization (and thus, 
> for travis integration).
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/importjob/DatabaseAdapterFactory.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/NumericTypesImportTest.java af310cbe2 
>   src/test/org/apache/sqoop/importjob/SplitByImportTest.java 90b7cbbd3 
>   
> src/test/org/apache/sqoop/importjob/configuration/MSSQLServerImportJobTestConfiguration.java
>  4ad7defe1 
>   
> src/test/org/apache/sqoop/importjob/configuration/MySQLImportJobTestConfiguration.java
>  fbcbdebeb 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/MysqlNumericTypesImportTest.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/PostgresNumericTypesImportTest.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/SqlServerNumericTypesImportTest.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/MysqlSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/OracleSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/PostgresSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/SqlServerSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
> 22567162d 
>   src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java 
> ebd014688 
> 
> 
> Diff: https://reviews.apache.org/r/69407/diff/7/
> 
> 
> Testing
> ---
> 
> unit and 3rd party tests.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/
---

(Updated Nov. 22, 2018, 1:39 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3405
https://issues.apache.org/jira/browse/SQOOP-3405


Repository: sqoop-trunk


Description
---

Breaking up the parameterized test classes into a per database basis. Provides 
better readability, needed for proper test categorization (and thus, for travis 
integration).


Diffs (updated)
-

  src/test/org/apache/sqoop/importjob/DatabaseAdapterFactory.java PRE-CREATION 
  src/test/org/apache/sqoop/importjob/NumericTypesImportTest.java af310cbe2 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java 90b7cbbd3 
  
src/test/org/apache/sqoop/importjob/configuration/MSSQLServerImportJobTestConfiguration.java
 4ad7defe1 
  
src/test/org/apache/sqoop/importjob/configuration/MySQLImportJobTestConfiguration.java
 fbcbdebeb 
  
src/test/org/apache/sqoop/importjob/numerictypes/MysqlNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/PostgresNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/SqlServerNumericTypesImportTest.java
 PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/MysqlSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/OracleSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/PostgresSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/SqlServerSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
22567162d 
  src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java 
ebd014688 


Diff: https://reviews.apache.org/r/69407/diff/7/

Changes: https://reviews.apache.org/r/69407/diff/6-7/


Testing
---

unit and 3rd party tests.


Thanks,

Fero Szabo

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

> On Nov. 21, 2018, 2:05 p.m., Boglarka Egyed wrote:
> > src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
> > Lines 1 (patched)
> > 
> >
> > Apache headers are missing from new files.

Thanks for pointing this out, should be ok now. I didn't make any other code 
change, just added the licence headers, but Reviewboard somehow shows more...


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/#review210762
---


On Nov. 21, 2018, 3 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69407/
> ---
> 
> (Updated Nov. 21, 2018, 3 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3405
> https://issues.apache.org/jira/browse/SQOOP-3405
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Breaking up the parameterized test classes into a per database basis. 
> Provides better readability, needed for proper test categorization (and thus, 
> for travis integration).
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/importjob/DatabaseAdapterFactory.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/NumericTypesImportTest.java af310cb 
>   src/test/org/apache/sqoop/importjob/SplitByImportTest.java 90b7cbb 
>   
> src/test/org/apache/sqoop/importjob/configuration/MSSQLServerImportJobTestConfiguration.java
>  4ad7def 
>   
> src/test/org/apache/sqoop/importjob/configuration/MySQLImportJobTestConfiguration.java
>  fbcbdeb 
>   
> src/test/org/apache/sqoop/importjob/configuration/MysqlImportJobTestConfiguration.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/configuration/SqlServerImportJobTestConfiguration.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/MysqlNumericTypesImportTest.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/NumericTypesImportTestBase.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/PostgresNumericTypesImportTest.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/numerictypes/SqlServerNumericTypesImportTest.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/MysqlSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/OracleSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/PostgresSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/SplitByImportTestBase.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/importjob/splitby/SqlServerSplitByImportTest.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
> 2256716 
>   src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java 
> ebd0146 
>   src/test/org/apache/sqoop/testutil/adapter/MysqlDatabaseAdapter.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/adapter/SqlServerDatabaseAdapter.java 
> PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69407/diff/6/
> 
> 
> Testing
> ---
> 
> unit and 3rd party tests.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/
---

(Updated Nov. 21, 2018, 3 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3405
https://issues.apache.org/jira/browse/SQOOP-3405


Repository: sqoop-trunk


Description
---

Breaking up the parameterized test classes into a per database basis. Provides 
better readability, needed for proper test categorization (and thus, for travis 
integration).


Diffs (updated)
-

  src/test/org/apache/sqoop/importjob/DatabaseAdapterFactory.java PRE-CREATION 
  src/test/org/apache/sqoop/importjob/NumericTypesImportTest.java af310cb 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java 90b7cbb 
  
src/test/org/apache/sqoop/importjob/configuration/MSSQLServerImportJobTestConfiguration.java
 4ad7def 
  
src/test/org/apache/sqoop/importjob/configuration/MySQLImportJobTestConfiguration.java
 fbcbdeb 
  
src/test/org/apache/sqoop/importjob/configuration/MysqlImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/configuration/SqlServerImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/MysqlNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/NumericTypesImportTestBase.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/PostgresNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/SqlServerNumericTypesImportTest.java
 PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/MysqlSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/OracleSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/PostgresSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/SplitByImportTestBase.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/SqlServerSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
2256716 
  src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java ebd0146 
  src/test/org/apache/sqoop/testutil/adapter/MysqlDatabaseAdapter.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/SqlServerDatabaseAdapter.java 
PRE-CREATION 


Diff: https://reviews.apache.org/r/69407/diff/6/

Changes: https://reviews.apache.org/r/69407/diff/5-6/


Testing
---

unit and 3rd party tests.


Thanks,

Fero Szabo

Re: Review Request 69414: Sqoop should not try to execute test category interfaces as tests with Ant

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69414/#review210763
---


Ship it!




Ship It!

- Fero Szabo


On Nov. 21, 2018, 12:22 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69414/
> ---
> 
> (Updated Nov. 21, 2018, 12:22 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3406
> https://issues.apache.org/jira/browse/SQOOP-3406
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> When Ant third party test suite is being run Ant tries to execute the test 
> category interfaces too because they end with the 'Test' postfix.
> 
> These "tests" obviously fail so we need to make sure that Ant does not 
> execute them.
> 
> 
> Diffs
> -
> 
>   build.xml 995a513040f85b6c2043a977a09e93b56913bbed 
> 
> 
> Diff: https://reviews.apache.org/r/69414/diff/2/
> 
> 
> Testing
> ---
> 
> ant unit and third party test
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 69413: Introduce methods instead of TEMP_BASE_DIR and LOCAL_WAREHOUSE_DIR static fields

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69413/#review210754
---


Ship it!




Lgtm.

- Fero Szabo


On Nov. 20, 2018, 5:29 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69413/
> ---
> 
> (Updated Nov. 20, 2018, 5:29 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3407
> https://issues.apache.org/jira/browse/SQOOP-3407
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> BaseSqoopTestCase.TEMP_BASE_DIR and BaseSqoopTestCase.LOCAL_WAREHOUSE_DIR are 
> public static fields which get initialized once at the JVM startup and store 
> the paths for the test temp and warehouse directories.
> 
> The problem is that HBase test cases change the value of the test.build.data 
> system property which can cause tests using these static fields to fail.
> 
> Since we do not own the code in HBase which changes the system property we 
> need to turn these static fields into methods which evaluate the 
> test.build.data system property every time they invoked which will make sure 
> that the invoking tests will be successful.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 
> dbdd05c13e77af514bd996a92f7ebea3a27aedd5 
>   src/test/org/apache/sqoop/TestMerge.java 
> b283174b8b3df7c16c496795fcbae2f91dd1c375 
>   src/test/org/apache/sqoop/credentials/TestPassingSecurePassword.java 
> 9c1e9f9a93323655bc313303bf84d566b551ee00 
>   src/test/org/apache/sqoop/hbase/HBaseImportAddRowKeyTest.java 
> df1840b37ce29ffb303b31e1fcbfe4c5842e7c36 
>   src/test/org/apache/sqoop/io/TestSplittableBufferedWriter.java 
> 71d6971489e489ae501739fdad5a7409375b6ec1 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
> ea7942f62d623895f242e69e77cf9920bbb7e18c 
>   src/test/org/apache/sqoop/orm/TestClassWriter.java 
> 59a8908f13c51b9caca42e8602413ee0b8634b0a 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java 
> e23aad3ee997780e5708e9180550339d834b74d9 
> 
> 
> Diff: https://reviews.apache.org/r/69413/diff/1/
> 
> 
> Testing
> ---
> 
> Executed unit and third party tests.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 69414: Sqoop should not try to execute test category interfaces as tests with Ant

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69414/#review210753
---




build.xml
Lines 950 (patched)


CubridTest.java is excluded twice?


- Fero Szabo


On Nov. 20, 2018, 5:32 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69414/
> ---
> 
> (Updated Nov. 20, 2018, 5:32 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3406
> https://issues.apache.org/jira/browse/SQOOP-3406
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> When Ant third party test suite is being run Ant tries to execute the test 
> category interfaces too because they end with the 'Test' postfix.
> 
> These "tests" obviously fail so we need to make sure that Ant does not 
> execute them.
> 
> 
> Diffs
> -
> 
>   build.xml 995a513040f85b6c2043a977a09e93b56913bbed 
> 
> 
> Diff: https://reviews.apache.org/r/69414/diff/1/
> 
> 
> Testing
> ---
> 
> ant unit and third party test
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/
---

(Updated Nov. 21, 2018, 10:20 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3405
https://issues.apache.org/jira/browse/SQOOP-3405


Repository: sqoop-trunk


Description
---

Breaking up the parameterized test classes into a per database basis. Provides 
better readability, needed for proper test categorization (and thus, for travis 
integration).


Diffs (updated)
-

  src/test/org/apache/sqoop/importjob/DatabaseAdapterFactory.java PRE-CREATION 
  src/test/org/apache/sqoop/importjob/NumericTypesImportTest.java af310cbe 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java 90b7cbbd 
  
src/test/org/apache/sqoop/importjob/configuration/MSSQLServerImportJobTestConfiguration.java
 4ad7defe 
  
src/test/org/apache/sqoop/importjob/configuration/MySQLImportJobTestConfiguration.java
 fbcbdebe 
  
src/test/org/apache/sqoop/importjob/numerictypes/MysqlNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/PostgresNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/SqlServerNumericTypesImportTest.java
 PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/MysqlSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/OracleSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/PostgresSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/SqlServerSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
22567162 
  src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java ebd01468 


Diff: https://reviews.apache.org/r/69407/diff/5/

Changes: https://reviews.apache.org/r/69407/diff/4-5/


Testing
---

unit and 3rd party tests.


Thanks,

Fero Szabo

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/
---

(Updated Nov. 21, 2018, 9:31 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3405
https://issues.apache.org/jira/browse/SQOOP-3405


Repository: sqoop-trunk


Description
---

Breaking up the parameterized test classes into a per database basis. Provides 
better readability, needed for proper test categorization (and thus, for travis 
integration).


Diffs (updated)
-

  src/test/org/apache/sqoop/importjob/DatabaseAdapterFactory.java PRE-CREATION 
  src/test/org/apache/sqoop/importjob/NumericTypesImportTest.java af310cbe2 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java 90b7cbbd3 
  
src/test/org/apache/sqoop/importjob/configuration/MSSQLServerImportJobTestConfiguration.java
 4ad7defe1 
  
src/test/org/apache/sqoop/importjob/configuration/MySQLImportJobTestConfiguration.java
 fbcbdebeb 
  
src/test/org/apache/sqoop/importjob/numerictypes/MysqlNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/PostgresNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/SqlServerNumericTypesImportTest.java
 PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/MysqlSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/OracleSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/PostgresSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/SqlServerSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
22567162d 
  src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java 
ebd014688 


Diff: https://reviews.apache.org/r/69407/diff/4/

Changes: https://reviews.apache.org/r/69407/diff/3-4/


Testing
---

unit and 3rd party tests.


Thanks,

Fero Szabo

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/
---

(Updated Nov. 21, 2018, 9:29 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3405
https://issues.apache.org/jira/browse/SQOOP-3405


Repository: sqoop-trunk


Description
---

Breaking up the parameterized test classes into a per database basis. Provides 
better readability, needed for proper test categorization (and thus, for travis 
integration).


Diffs (updated)
-

  src/java/org/apache/sqoop/hive/HiveTypes.java 554a03608 
  src/java/org/apache/sqoop/hive/TableDefWriter.java b21dfe534 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java 8cfd776d5 


Diff: https://reviews.apache.org/r/69407/diff/3/

Changes: https://reviews.apache.org/r/69407/diff/2-3/


Testing
---

unit and 3rd party tests.


Thanks,

Fero Szabo

Re: Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/
---

(Updated Nov. 21, 2018, 9:07 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

Refactored test class names to the same as used in the categorization


Bugs: SQOOP-3405
https://issues.apache.org/jira/browse/SQOOP-3405


Repository: sqoop-trunk


Description
---

Breaking up the parameterized test classes into a per database basis. Provides 
better readability, needed for proper test categorization (and thus, for travis 
integration).


Diffs (updated)
-

  src/java/org/apache/sqoop/hive/HiveTypes.java 554a03608 
  src/java/org/apache/sqoop/hive/TableDefWriter.java b21dfe534 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java 8cfd776d5 


Diff: https://reviews.apache.org/r/69407/diff/2/

Changes: https://reviews.apache.org/r/69407/diff/1-2/


Testing
---

unit and 3rd party tests.


Thanks,

Fero Szabo

Review Request 69407: Refactor: break up Parameterized tests on a per database basis

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69407/
---

Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3405
https://issues.apache.org/jira/browse/SQOOP-3405


Repository: sqoop-trunk


Description
---

Breaking up the parameterized test classes into a per database basis. Provides 
better readability, needed for proper test categorization (and thus, for travis 
integration).


Diffs
-

  src/test/org/apache/sqoop/importjob/DatabaseAdapterFactory.java PRE-CREATION 
  src/test/org/apache/sqoop/importjob/NumericTypesImportTest.java af310cbe2 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java 90b7cbbd3 
  
src/test/org/apache/sqoop/importjob/configuration/MSSQLServerImportJobTestConfiguration.java
 4ad7defe1 
  
src/test/org/apache/sqoop/importjob/configuration/MySQLImportJobTestConfiguration.java
 fbcbdebeb 
  
src/test/org/apache/sqoop/importjob/numerictypes/MysqlNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/OracleNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/PostgresNumericTypesImportTest.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/numerictypes/SqlServerNumericTypesImportTest.java
 PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/MysqlSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/OracleSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/PostgresSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/importjob/splitby/SqlServerSplitByImportTest.java 
PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
22567162d 
  src/test/org/apache/sqoop/testutil/adapter/MySqlDatabaseAdapter.java 
ebd014688 


Diff: https://reviews.apache.org/r/69407/diff/1/


Testing
---

unit and 3rd party tests.


Thanks,

Fero Szabo

Re: Review Request 69346: Categorize all tests in the project

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69346/#review210576
---


Ship it!




Hi Szabi,

lgtm, just a minor suggestion.


COMPILING.txt
Line 465 (original), 633 (patched)


I believe you require a fully qualified classname here, right? A concrete 
example here might come in handy. What do you think?


- Fero Szabo


On Nov. 15, 2018, 5:45 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69346/
> ---
> 
> (Updated Nov. 15, 2018, 5:45 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3404
> https://issues.apache.org/jira/browse/SQOOP-3404
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> - All tests are categorized now
> - Introduced S3Test category as a subcategory of ThirdPartyTest
> - Reorganized test tasks: we have unitTest, integrationTest, kerberizedTest, 
> test, s3Test, allTest tasks now
> - jacocoTestReport task is fixed to contain the coverage information of the 
> kerberizedTest task too. This is needed because the kerberizedTest needs the 
> forkEvery parameter to be set to 1 and because of that it has to be a 
> separate task which generates separate coverage information too. However it 
> is automatically triggered after the test task so the invocation is more 
> convenient for the tester.
> 
> 
> Diffs
> -
> 
>   COMPILING.txt 835ba33b1e89158bed0e05698b188ab3323eb881 
>   build.gradle cb9eeca74bbf278c3e5fd15de608d8c37c917ddb 
>   src/test/org/apache/sqoop/importjob/SplitByImportTest.java 
> c6fe4f2e8a80c96ad667f4fe4a26510af96562dc 
>   src/test/org/apache/sqoop/manager/TestMainframeManager.java 
> c84f05f660c396a06a5031e00abdae77ffbcf2aa 
>   
> src/test/org/apache/sqoop/manager/oracle/TestOraOopDBInputSplitGetDebugDetails.java
>  6f33ad3b650436b7f268b4ef5bfd451bd5e6958e 
>   
> src/test/org/apache/sqoop/mapreduce/db/netezza/TestNetezzaExternalTableExportMapper.java
>  5e558717c0d43301ecbf81a37d5ee3fd35756d65 
>   
> src/test/org/apache/sqoop/mapreduce/db/netezza/TestNetezzaExternalTableImportMapper.java
>  1a6943786834d27f27523f484d76cf678f18cf48 
>   
> src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
>  b4cba28c3611400b5c4227a5166b6c91e9152dc4 
>   
> src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileGdgEntryParser.java
>  521a04266e8806321fe7aa6a89c064f369174523 
>   src/test/org/apache/sqoop/s3/TestS3AvroImport.java 
> 7f5f5d62c5cab10f932aa22c3a713b13fefc2b58 
>   src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java 
> 0c3161e5a783446e35f4754124f86715d103ec0b 
>   src/test/org/apache/sqoop/s3/TestS3ImportWithHadoopCredProvider.java 
> 3a0d6365dc20f8eef5bdd67a4a2dc9c68ff74d7f 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendAvroImport.java 
> 5faf59ea80c48fe025294cabd100e7d176032138 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendParquetImport.java 
> a4f986423ea299716a29f9d02f7c8453a7f2ba02 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendSequenceFileImport.java 
> d271588c5af060bbc3d301a845f45c46d0f6a2ba 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendTextImport.java 
> 52d89c775b5f1219471df44d222fd92a59ed408c 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeParquetImport.java 
> 39238c5fab56b54a85dde5aed0d4bb2c77382fa6 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeTextImport.java 
> 597e3def2cc33adebeeb3bc1ee35ad8a7f4b990d 
>   src/test/org/apache/sqoop/s3/TestS3ParquetImport.java 
> c9785d816d4a7a5870d74c51a9faa229f6d3818e 
>   src/test/org/apache/sqoop/s3/TestS3SequenceFileImport.java 
> bba8b74ebe639df26e977abf377f4904144dcfaa 
>   src/test/org/apache/sqoop/s3/TestS3TextImport.java 
> 114f97cbb8857a7633cae5d030769ac4a90e36aa 
>   src/test/org/apache/sqoop/testcategories/thirdpartytest/S3Test.java 
> PRE-CREATION 
>   
> src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java 
> 7745f1b07e6d6c457b0164deeace12587ec058d0 
> 
> 
> Diff: https://reviews.apache.org/r/69346/diff/3/
> 
> 
> Testing
> ---
> 
> ./gradlew unitTest
> ./gradlew integrationTest
> ./gradlew kerberizedTest
> ./gradlew ... s3Test
> ./gradlew test
> ./gradlew ... thirdPartyTest
> ./gradlew allTest
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/
---

(Updated Nov. 12, 2018, 4:33 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3382
https://issues.apache.org/jira/browse/SQOOP-3382


Repository: sqoop-trunk


Description
---

This patch is about adding support for fixed point decimal types in parquet 
import.

The implementation is simple after the fact that parquet was upgraded to 1.9.0 
in SQOOP-3381: we just need to register the GenericDataSupplier with 
AvroParquetOutputFormat.

For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
under the hood to write parquet.

I also moved around and renamed the classes involved in this change so their 
name and package reflect their purpose.

** Note: A key design decision can be seen in the ImportJobTestConfiguration 
interface **
- I decided to create a new function to get the expected results for each file 
format, since we seldom add new fileformats. 
- However this also enforces future configurations to always define their 
expected result for every file forma or throw a NotImplementedException should 
they lack the support for one.
- The alternative for this is to define the fileLayout as an input parameter 
instead. This would allow for better extendability.
_Please share your thoughts on this!_


Diffs (updated)
-

  src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250e 
  src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8ab 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
 e82154309 
  src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9cd 
  src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 14de910b9 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java 7977c0b0f 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3bc 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
 182d2967f 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
 e9bf9912a 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
 b7bad08c0 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
 465e61f4b 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
 66715c171 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 ec4db41bd 
  src/test/org/apache/sqoop/importjob/configuration/AvroTestConfiguration.java 
PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
 f137b56b7 
  
src/test/org/apache/sqoop/importjob/configuration/ParquetTestConfiguration.java 
PRE-CREATION 
  src/test/org/apache/sqoop/util/ParquetReader.java 908ce566f 


Diff: https://reviews.apache.org/r/69060/diff/4/

Changes: https://reviews.apache.org/r/69060/diff/3-4/


Testing
---

3rd party tests and unit tests, both gradle and ant


Thanks,

Fero Szabo

Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

[Fero Szabo via Review Board]

> On Nov. 9, 2018, 2:26 p.m., Boglarka Egyed wrote:
> > src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java
> > Lines 220 (patched)
> > 
> >
> > I think these tests could be parameterized as they are doing the same 
> > but with different file formats (Avro and Parquet).

Hi Bogi,

Thanks for the review!

There is a tiny difference: to enable logical types in parquet, there is new 
flag (sqoop.parquet.logical_types.decimal.enable), i.e. only used in the 
parquet tests. 

I'd keep this code as is, as deduplication might lead to spaghetti code here 
(since these are different features after all).

Even though this is a bit of a compromise, I'd like to drop this issue if 
that's OK with you (?)


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/#review210439
---


On Nov. 8, 2018, 3:34 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69060/
> ---
> 
> (Updated Nov. 8, 2018, 3:34 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3382
> https://issues.apache.org/jira/browse/SQOOP-3382
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This patch is about adding support for fixed point decimal types in parquet 
> import.
> 
> The implementation is simple after the fact that parquet was upgraded to 
> 1.9.0 in SQOOP-3381: we just need to register the GenericDataSupplier with 
> AvroParquetOutputFormat.
> 
> For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
> under the hood to write parquet.
> 
> I also moved around and renamed the classes involved in this change so their 
> name and package reflect their purpose.
> 
> ** Note: A key design decision can be seen in the ImportJobTestConfiguration 
> interface **
> - I decided to create a new function to get the expected results for each 
> file format, since we seldom add new fileformats. 
> - However this also enforces future configurations to always define their 
> expected result for every file forma or throw a NotImplementedException 
> should they lack the support for one.
> - The alternative for this is to define the fileLayout as an input parameter 
> instead. This would allow for better extendability.
> _Please share your thoughts on this!_
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250e 
>   src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c069888 
>   src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8ab 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9cd 
>   src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 
> 14de910b9 
>   src/test/org/apache/sqoop/importjob/SplitByImportTest.java 7977c0b0f 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3bc 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
>  182d2967f 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
>  e9bf9912a 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
>  b7bad08c0 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
>  465e61f4b 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
>  66715c171 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
>  ec4db41bd 
>   
> src/test/org/apache/sqoop/importjob/configuration/AvroTestConfiguration.java 
> PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
>  f137b56b7 
>   
> src/test/org/apache/sqoop/importjob/configuration/ParquetTestConfiguration.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/util/ParquetReader.java 908ce566f 
> 
> 
> Diff: https://reviews.apache.org/r/69060/diff/3/
> 
> 
> Testing
> ---
> 
> 3rd party tests and unit tests, both gradle and ant
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/
---

(Updated Nov. 8, 2018, 3:34 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

The most notable change in this newest changesetis around verification: 
- as we discussed with Szabi: since decimal conversion is put in place it's not 
enough to assert on the data itself (as the data appears as String in-memory, 
it could be String on the disk as well)
- so, I've added a new function that checks the schema for decimal types as well


Bugs: SQOOP-3382
https://issues.apache.org/jira/browse/SQOOP-3382


Repository: sqoop-trunk


Description
---

This patch is about adding support for fixed point decimal types in parquet 
import.

The implementation is simple after the fact that parquet was upgraded to 1.9.0 
in SQOOP-3381: we just need to register the GenericDataSupplier with 
AvroParquetOutputFormat.

For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
under the hood to write parquet.

I also moved around and renamed the classes involved in this change so their 
name and package reflect their purpose.

** Note: A key design decision can be seen in the ImportJobTestConfiguration 
interface **
- I decided to create a new function to get the expected results for each file 
format, since we seldom add new fileformats. 
- However this also enforces future configurations to always define their 
expected result for every file forma or throw a NotImplementedException should 
they lack the support for one.
- The alternative for this is to define the fileLayout as an input parameter 
instead. This would allow for better extendability.
_Please share your thoughts on this!_


Diffs (updated)
-

  src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250e 
  src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c069888 
  src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8ab 
  src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9cd 
  src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 14de910b9 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java 7977c0b0f 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3bc 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
 182d2967f 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
 e9bf9912a 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
 b7bad08c0 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
 465e61f4b 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
 66715c171 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 ec4db41bd 
  src/test/org/apache/sqoop/importjob/configuration/AvroTestConfiguration.java 
PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
 f137b56b7 
  
src/test/org/apache/sqoop/importjob/configuration/ParquetTestConfiguration.java 
PRE-CREATION 
  src/test/org/apache/sqoop/util/ParquetReader.java 908ce566f 


Diff: https://reviews.apache.org/r/69060/diff/3/

Changes: https://reviews.apache.org/r/69060/diff/2-3/


Testing
---

3rd party tests and unit tests, both gradle and ant


Thanks,

Fero Szabo

Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

[Fero Szabo via Review Board]

> On Oct. 26, 2018, 7:40 a.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java
> > Lines 292 (patched)
> > 
> >
> > Since the readAll* methods of ParquetReader close the reader this 
> > method could be simplified to something like this:
> > 
> > private void verifyParquetFile() {
> > ParquetReader reader = new ParquetReader(new Path(getWarehouseDir() 
> > + "/" + getTableName()), getConf());
> > assertEquals(asList(configuration.getExpectedResultsForParquet()), 
> > reader.readAllInCsv());
> >   }

I think this no longer applies since I access the convertToCsv method 4 lines 
later of the same reader.
That method could be made public static as well, though I'm unsure of such a 
utility method would be considered best practice. (?)


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/#review210083
---


On Nov. 8, 2018, 3:34 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69060/
> ---
> 
> (Updated Nov. 8, 2018, 3:34 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3382
> https://issues.apache.org/jira/browse/SQOOP-3382
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This patch is about adding support for fixed point decimal types in parquet 
> import.
> 
> The implementation is simple after the fact that parquet was upgraded to 
> 1.9.0 in SQOOP-3381: we just need to register the GenericDataSupplier with 
> AvroParquetOutputFormat.
> 
> For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
> under the hood to write parquet.
> 
> I also moved around and renamed the classes involved in this change so their 
> name and package reflect their purpose.
> 
> ** Note: A key design decision can be seen in the ImportJobTestConfiguration 
> interface **
> - I decided to create a new function to get the expected results for each 
> file format, since we seldom add new fileformats. 
> - However this also enforces future configurations to always define their 
> expected result for every file forma or throw a NotImplementedException 
> should they lack the support for one.
> - The alternative for this is to define the fileLayout as an input parameter 
> instead. This would allow for better extendability.
> _Please share your thoughts on this!_
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250e 
>   src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c069888 
>   src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8ab 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9cd 
>   src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 
> 14de910b9 
>   src/test/org/apache/sqoop/importjob/SplitByImportTest.java 7977c0b0f 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3bc 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
>  182d2967f 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
>  e9bf9912a 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
>  b7bad08c0 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
>  465e61f4b 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
>  66715c171 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
>  ec4db41bd 
>   
> src/test/org/apache/sqoop/importjob/configuration/AvroTestConfiguration.java 
> PRE-CREATION 
>   
> src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
>  f137b56b7 
>   
> src/test/org/apache/sqoop/importjob/configuration/ParquetTestConfiguration.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/util/ParquetReader.java 908ce566f 
> 
> 
> Diff: https://reviews.apache.org/r/69060/diff/3/
> 
> 
> Testing
> ---
> 
> 3rd party tests and unit tests, both gradle and ant
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 69199: Create tests for SQOOP-2949, quote escaping in split-by

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69199/
---

(Updated Oct. 31, 2018, 1:51 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3400
https://issues.apache.org/jira/browse/SQOOP-3400


Repository: sqoop-trunk


Description
---

Integration tests for SQOOP-2949.


Diffs (updated)
-

  src/java/org/apache/sqoop/mapreduce/db/TextSplitter.java 22bbfe68 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/69199/diff/6/

Changes: https://reviews.apache.org/r/69199/diff/5-6/


Testing
---

This is the testing part for a fix that lacked testing. 
gradle test and gradle 3rdpartytests.


Thanks,

Fero Szabo

Re: Review Request 69199: Create tests for SQOOP-2949, quote escaping in split-by

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69199/
---

(Updated Oct. 31, 2018, 12:54 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

Implemented fixes.


Bugs: SQOOP-3400
https://issues.apache.org/jira/browse/SQOOP-3400


Repository: sqoop-trunk


Description
---

Integration tests fro SQOOP-2949.


Diffs (updated)
-

  src/java/org/apache/sqoop/mapreduce/db/TextSplitter.java 22bbfe68 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/69199/diff/4/

Changes: https://reviews.apache.org/r/69199/diff/3-4/


Testing
---

This is the testing part for a fix that lacked testing. 
gradle test and gradle 3rdpartytests.


Thanks,

Fero Szabo

Re: Review Request 69199: Create tests for SQOOP-2949, quote escaping in split-by

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69199/
---

(Updated Oct. 30, 2018, 4:26 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

As we discussed offline with Szabolcs, I've updated this diff to include the 
fix for SQOOP-2949.

Also, simplified tests a bit by asserting for Parquet output.


Bugs: SQOOP-3400
https://issues.apache.org/jira/browse/SQOOP-3400


Repository: sqoop-trunk


Description
---

Integration tests fro SQOOP-2949.


Diffs (updated)
-

  src/java/org/apache/sqoop/mapreduce/db/TextSplitter.java 22bbfe68 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
 PRE-CREATION 
  src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java fe6ba831 


Diff: https://reviews.apache.org/r/69199/diff/2/

Changes: https://reviews.apache.org/r/69199/diff/1-2/


Testing
---

This is the testing part for a fix that lacked testing. 
gradle test and gradle 3rdpartytests.


Thanks,

Fero Szabo

Review Request 69199: Create tests for SQOOP-2949, quote escaping in split-by

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69199/
---

Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3400
https://issues.apache.org/jira/browse/SQOOP-3400


Repository: sqoop-trunk


Description
---

Integration tests fro SQOOP-2949.


Diffs
-

  src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 14de910b 
  src/test/org/apache/sqoop/importjob/SplitByImportTest.java PRE-CREATION 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3b 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
 182d2967 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
 e9bf9912 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
 b7bad08c 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
 465e61f4 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
 66715c17 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 ec4db41b 
  
src/test/org/apache/sqoop/importjob/configuration/GenericImportJobSplitByTestConfiguration.java
 PRE-CREATION 
  src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java fe6ba831 


Diff: https://reviews.apache.org/r/69199/diff/1/


Testing
---

This is the testing part for a fix that lacked testing. 
gradle test and gradle 3rdpartytests.


Thanks,

Fero Szabo

Review Request 69200: SQL Syntax error when split-by column is of character type and min or max value has single quote inside it

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69200/
---

Review request for Sqoop, Boglarka Egyed, Fero Szabo, and Szabolcs Vasas.


Bugs: SQOOP-2949
https://issues.apache.org/jira/browse/SQOOP-2949


Repository: sqoop-trunk


Description
---

This fix was developped by Gireesh Puthumana.

This is a fix for text splitting. 
- Previously, if a split boundary record had a quote in inside, an error 
occured because of the lack of escaping. 
- This change adds the escaping for quotes via standard SQL notation that 
should work across all databases.


Diffs
-

  src/java/org/apache/sqoop/mapreduce/db/TextSplitter.java 22bbfe68 


Diff: https://reviews.apache.org/r/69200/diff/1/


Testing
---

New integration tests were developed in SQOOP-3400.


Thanks,

Fero Szabo

Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

[Fero Szabo via Review Board]

> On Oct. 24, 2018, 2:31 p.m., Szabolcs Vasas wrote:
> > Hi Feró,
> > 
> > Thank you for submitting this improvement!
> > I have left some comments, see them below.
> > Apart from that I think we need to test explicitly that if the 
> > sqoop.parquet.logical_types.decimal.enable flag is true then the Parquet 
> > file contains a decimal value and otherwise it contains a string value.
> > 
> > NumericTypesImportTest asserts on string values so it is not able to verify 
> > this, most of the tests passed even if I commented out the content of the 
> > addEnableParquetDecimal method.

I'll look into this one.

I'm thinking that using the org.apache.sqoop.util.ParquetReader#readAll method 
could help (since it returns GenericRecords), though I'm not sure. I'll somehow 
need to actually turn off the conversion and check for the bytes. Any 
suggestions?


> On Oct. 24, 2018, 2:31 p.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/mapreduce/ImportJobBase.java
> > Lines 115-119 (patched)
> > 
> >
> > Is it possible to move this to 
> > org.apache.sqoop.mapreduce.parquet.hadoop.HadoopParquetImportJobConfigurator#configureMapper?
> > That would be consistent with the way we configure the Parquet imports 
> > but I am not sure the effect would remain the same.

Yes, makes sense.


> On Oct. 24, 2018, 2:31 p.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java
> > Lines 56 (patched)
> > 
> >
> > Are we sure that adding the logical type conversion only here is enough?
> > In case of Avro it is also added in 
> > org.apache.sqoop.mapreduce.AvroOutputFormat#getRecordWriter which gets 
> > invoked in every mapper so I assume that we have to add the conversion in 
> > every mapper in case of Parquet files too.

My understanding is that this method is invoked in every mapper. (it's doc 
suggest this as well: "Called once at the beginning of the task.")
Where else would put this statement?


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/#review209954
---


On Oct. 24, 2018, 12:25 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69060/
> ---
> 
> (Updated Oct. 24, 2018, 12:25 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3382
> https://issues.apache.org/jira/browse/SQOOP-3382
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This patch is about adding support for fixed point decimal types in parquet 
> import.
> 
> The implementation is simple after the fact that parquet was upgraded to 
> 1.9.0 in SQOOP-3381: we just need to register the GenericDataSupplier with 
> AvroParquetOutputFormat.
> 
> For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
> under the hood to write parquet.
> 
> I also moved around and renamed the classes involved in this change so their 
> name and package reflect their purpose.
> 
> ** Note: A key design decision can be seen in the ImportJobTestConfiguration 
> interface **
> - I decided to create a new function to get the expected results for each 
> file format, since we seldom add new fileformats. 
> - However this also enforces future configurations to always define their 
> expected result for every file forma or throw a NotImplementedException 
> should they lack the support for one.
> - The alternative for this is to define the fileLayout as an input parameter 
> instead. This would allow for better extendability.
> _Please share your thoughts on this!_
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250 
>   src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c06988 
>   src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8a 
>   src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9c 
>   src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 
> 14de910b 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> ff13dc3b 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
>  182d2967 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
>  e9bf9912 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
>  b7bad08c 
>   
> src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
>  465e61f4 
>   
> 

Re: Review Request 69139: TestS3ImportWithHadoopCredProvider fails if credential generator command is not provided

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69139/#review209969
---


Ship it!




Lgtm. I've only run this one test though.

- Fero Szabo


On Oct. 24, 2018, 10:58 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69139/
> ---
> 
> (Updated Oct. 24, 2018, 10:58 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3399
> https://issues.apache.org/jira/browse/SQOOP-3399
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> BeforeClass method of TestS3ImportWithHadoopCredProvider should not throw 
> NullPointerException when the credential generator command is not provided 
> since it fails the test with Gradle.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/s3/TestS3ImportWithHadoopCredProvider.java 
> e03eb64ef 
> 
> 
> Diff: https://reviews.apache.org/r/69139/diff/1/
> 
> 
> Testing
> ---
> 
> Executed the test with both and and gradle, with and without S3 credential 
> generator provided.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 69066: SQOOP-3390: Document S3Guard usage with Sqoop

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69066/#review209964
---


Ship it!




Bogi, thanks for enhancing our documentation as well!

Both ant and gw docs are successful on my side, and the output look good.

- Fero Szabo


On Oct. 17, 2018, 4:49 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69066/
> ---
> 
> (Updated Oct. 17, 2018, 4:49 p.m.)
> 
> 
> Review request for Sqoop, Fero Szabo and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3390
> https://issues.apache.org/jira/browse/SQOOP-3390
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Document Hadoop's S3Guard usage with Sqoop to overcome Amazon S3's eventual 
> consistency.
> 
> 
> Diffs
> -
> 
>   src/docs/user/s3.txt c54b26bc5ef71f8cd7d18ce6eb98a296dbffed92 
> 
> 
> Diff: https://reviews.apache.org/r/69066/diff/1/
> 
> 
> Testing
> ---
> 
> ant docs
> ./gradlew docs
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 69141: Tests using HiveMiniCluster can be unstable on some platforms

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69141/#review209963
---


Ship it!




gradlew test and thirdpartytest are successful on my computer.

- Fero Szabo


On Oct. 24, 2018, 12:25 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69141/
> ---
> 
> (Updated Oct. 24, 2018, 12:25 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3398
> https://issues.apache.org/jira/browse/SQOOP-3398
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Since the last Hive upgrade TestHiveMiniCluster fails on some platforms 
> because and older version of the ASM library is picked up.
> 
> The task is to exclude the older ASM library in ivy and gradle to make sure 
> the test passes on all platforms.
> 
> 
> Diffs
> -
> 
>   build.gradle 2340bce7519a46b203a287a4b5160c62e0c09509 
>   ivy.xml 6805fc329d44bcc0707e7cab67f3749a42e6f769 
> 
> 
> Diff: https://reviews.apache.org/r/69141/diff/1/
> 
> 
> Testing
> ---
> 
> Executed unit and third party tests with both ant and gradle.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68064: SQOOP-3355 Document SQOOP-1905 DB2 --schema option

[Fero Szabo via Review Board]

> On Oct. 24, 2018, 12:21 p.m., Szabolcs Vasas wrote:
> > src/docs/user/connectors.txt
> > Lines 41 (patched)
> > 
> >
> > Are you sure import-all-tables supports --schema properly?
> > By looking at the code of this command it seems to me that Sqoop uses 
> > the list-tables command to get the table names it wants to import but then 
> > it invokes the import command in a loop to import all the tables. So I 
> > think that even if the list-tables shows a table the import command will 
> > not be able to pick it up.

Hi Szabi,

Yes, I've tested this in July, when I created the review. Now this reminds me 
to write down everything. :)

Anyway, I've tested again, and this works as expected. What I did was: fire up 
a cluster, created a couple of tables in a DB2 instance and ran the command. 2 
different schemas, all the data form all of the tables are in hdfs. I'd say 
this is proper behavior.

Now I'm not sure why/how this feature wasn't implemented for a (simple) import, 
but it just wasn't. The error message suggests that sqoop tries to use the 
default schema, i.e. DB2INST1. We can open a Jira if there isn't one.

I'm dropping this issue.


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68064/#review209952
---


On July 26, 2018, 2:58 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68064/
> ---
> 
> (Updated July 26, 2018, 2:58 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3355
> https://issues.apache.org/jira/browse/SQOOP-3355
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Adding documentation for a previously implemented feature. This one is quite 
> simple.
> 
> 
> Diffs
> -
> 
>   src/docs/user/connectors.txt 59e3e00b 
> 
> 
> Diff: https://reviews.apache.org/r/68064/diff/1/
> 
> 
> Testing
> ---
> 
> ant docs, 
> + unit and 3rd party tests, though these shouldn't be affected.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/
---

(Updated Oct. 24, 2018, 12:25 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3382
https://issues.apache.org/jira/browse/SQOOP-3382


Repository: sqoop-trunk


Description (updated)
---

This patch is about adding support for fixed point decimal types in parquet 
import.

The implementation is simple after the fact that parquet was upgraded to 1.9.0 
in SQOOP-3381: we just need to register the GenericDataSupplier with 
AvroParquetOutputFormat.

For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
under the hood to write parquet.

I also moved around and renamed the classes involved in this change so their 
name and package reflect their purpose.

** Note: A key design decision can be seen in the ImportJobTestConfiguration 
interface **
- I decided to create a new function to get the expected results for each file 
format, since we seldom add new fileformats. 
- However this also enforces future configurations to always define their 
expected result for every file forma or throw a NotImplementedException should 
they lack the support for one.
- The alternative for this is to define the fileLayout as an input parameter 
instead. This would allow for better extendability.
_Please share your thoughts on this!_


Diffs
-

  src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250 
  src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c06988 
  src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8a 
  src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9c 
  src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 14de910b 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3b 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
 182d2967 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
 e9bf9912 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
 b7bad08c 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
 465e61f4 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
 66715c17 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 ec4db41b 
  src/test/org/apache/sqoop/util/ParquetReader.java 908ce566 


Diff: https://reviews.apache.org/r/69060/diff/2/


Testing
---

3rd party tests and unit tests, both gradle and ant


Thanks,

Fero Szabo

Re: Review Request 69070: SQOOP-3394: External Hive table tests should use unique external dir names

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69070/#review209779
---


Ship it!




Lgtm

- Fero Szabo


On Oct. 18, 2018, 5:49 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69070/
> ---
> 
> (Updated Oct. 18, 2018, 5:49 p.m.)
> 
> 
> Review request for Sqoop, Fero Szabo and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3394
> https://issues.apache.org/jira/browse/SQOOP-3394
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Current external Hive table tests on S3 uses the same external directory name 
> in every unit test cases which can cause problems during running them in an 
> automated environment. These names should be unique in every test cases.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/testutil/S3TestUtils.java 
> 97d53bbaa7c72d2ad1b890d7a8367c45a3e2b95c 
> 
> 
> Diff: https://reviews.apache.org/r/69070/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew test -Ds3.bucket.url= 
> -Ds3.generator.command=
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 69060: SQOOP-3382 Add parquet numeric support for Parquet in hdfs import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/
---

(Updated Oct. 18, 2018, 5:40 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

Added a new flag for enabling logical types with parquet. This should allow 
backward compatibility.


Summary (updated)
-

SQOOP-3382 Add parquet numeric support for Parquet in hdfs import


Bugs: SQOOP-3382
https://issues.apache.org/jira/browse/SQOOP-3382


Repository: sqoop-trunk


Description
---

This patch is about adding support for fixed point decimal types in parquet 
import.

The implementation is simple after the fact that parquet was upgraded to 1.9.0 
in SQOOP-3381: we just need to register the GenericDataSupplier with 
AvroParquetOutputFormat.

For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
under the hood to write parquet.

I also moved around and renamed the classes involved in this change so their 
name and package reflect their purpose.


Diffs (updated)
-

  src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250 
  src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c06988 
  src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8a 
  src/java/org/apache/sqoop/orm/AvroSchemaGenerator.java 7a2a5f9c 
  src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 14de910b 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3b 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
 182d2967 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
 e9bf9912 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
 b7bad08c 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
 465e61f4 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
 66715c17 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 ec4db41b 
  src/test/org/apache/sqoop/util/ParquetReader.java 908ce566 


Diff: https://reviews.apache.org/r/69060/diff/2/

Changes: https://reviews.apache.org/r/69060/diff/1-2/


Testing
---

3rd party tests and unit tests, both gradle and ant


Thanks,

Fero Szabo

Re: Review Request 69063: SQOOP-3391: Test storing AWS credentials in Hadoop CredentialProvider during import

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69063/#review209740
---


Ship it!




lgtm

- Fero Szabo


On Oct. 18, 2018, 11:45 a.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69063/
> ---
> 
> (Updated Oct. 18, 2018, 11:45 a.m.)
> 
> 
> Review request for Sqoop, Fero Szabo, Ferenc Szabo, and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3391
> https://issues.apache.org/jira/browse/SQOOP-3391
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Test storing AWS credentials in Hadoop CredentialProvider during import in 
> case of
> - CredntialProvider with default password
> - CredntialProvider with password stored in environment variable
> - CredntialProvider with password file
> Added test cases for happy and sad paths as well.
> 
> Added a new test dependency both in Ant and Gradle for setting environment 
> variables in tests easily.
> 
> 
> Diffs
> -
> 
>   build.gradle 7a0712e3242e31ef2593c34f469f9136cf5dc85d 
>   build.xml f3975317140e66c700d85231669ccb2b70367f80 
>   conf/password-file.txt PRE-CREATION 
>   conf/wrong-password-file.txt PRE-CREATION 
>   gradle.properties 4808ec7d090b9732f9246f21e44bd736adf6efd0 
>   ivy.xml 91157ca74bee3b50269564ddb747638946e45a7e 
>   ivy/libraries.properties 2ca95ee99c09fe1aaff6797a6ee0958ac1977663 
>   src/java/org/apache/sqoop/util/password/CredentialProviderHelper.java 
> 1d6481a0697db2fc0ffeb1b012bb143beb615bc0 
>   src/test/org/apache/sqoop/s3/TestS3ImportWithHadoopCredProvider.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/S3TestUtils.java 
> c9d17bc728d6a229e32c157b56268d6418b3de94 
> 
> 
> Diff: https://reviews.apache.org/r/69063/diff/2/
> 
> 
> Testing
> ---
> 
> ./gradlew test -Ds3.bucket.url= 
> -Ds3.generator.command=
> ant clean test -Ds3.bucket.url= 
> -Ds3.generator.command=
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Review Request 69060: SQOOP-3382 Add parquet numeric support and reuse existing Avro numeric tests Parquet

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69060/
---

Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3382
https://issues.apache.org/jira/browse/SQOOP-3382


Repository: sqoop-trunk


Description
---

This patch is about adding support for fixed point decimal types in parquet 
import.

The implementation is simple after the fact that parquet was upgraded to 1.9.0 
in SQOOP-3381: we just need to register the GenericDataSupplier with 
AvroParquetOutputFormat.

For testing, we can reuse the existing Avro tests, because Sqoop uses Avro 
under the hood to write parquet.

I also moved around and renamed the classes involved in this change so their 
name and package reflect their purpose.


Diffs
-

  src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 80c06988 
  src/java/org/apache/sqoop/mapreduce/ParquetImportMapper.java 62334f8a 
  src/test/org/apache/sqoop/importjob/ImportJobTestConfiguration.java 14de910b 
  src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
ff13dc3b 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MSSQLServerImportJobTestConfiguration.java
 182d2967 
  
src/test/org/apache/sqoop/importjob/avro/configuration/MySQLImportJobTestConfiguration.java
 e9bf9912 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfiguration.java
 b7bad08c 
  
src/test/org/apache/sqoop/importjob/avro/configuration/OracleImportJobTestConfigurationForNumber.java
 465e61f4 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationForNumeric.java
 66715c17 
  
src/test/org/apache/sqoop/importjob/avro/configuration/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 ec4db41b 
  src/test/org/apache/sqoop/util/ParquetReader.java 908ce566 


Diff: https://reviews.apache.org/r/69060/diff/1/


Testing
---

3rd party tests and unit tests, both gradle and ant


Thanks,

Fero Szabo

Re: Review Request 68687: SQOOP-3381 Upgrade the Parquet library

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68687/
---

(Updated Oct. 16, 2018, 9:37 a.m.)


Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3381
https://issues.apache.org/jira/browse/SQOOP-3381


Repository: sqoop-trunk


Description
---

This change upgrades our parquet library to the newest version and a whole lot 
of libraries to newer versions with it.

As we will need to register a data supplier in the fix for parquet decimal 
support (SQOOP-3382), we will need a version that contains PARQUET-243. We need 
to upgrade the Parquet library to a version that contains this fix and is 
compatible with Hadoop 3.0.

A few things to note:
- hadoop's version is still 2.8.0
- hive is upgraded to 2.1.1
- the rest of the dependency changes are required for the hive version bump.

There is are a few changes in the codebase, but of course no new functionality 
at all:
- in the TestParquetImport class, the new implementation returns a Utf8 object 
for Strings written out.
- Added the security policy and related code changes from the patch for 
SQOOP-3305 (upgrade hadoop) written by Daniel Voros.
- modified HiveMiniCluster config so it won't try to start a web ui (it's 
unnecessary during tests anyway)


Diffs (updated)
-

  build.gradle fc7fc0c4 
  gradle.properties 0d30378d 
  gradle/sqoop-package.gradle 1a8d994d 
  ivy.xml 670cb32d 
  ivy/libraries.properties 8f3dab2b 
  src/java/org/apache/sqoop/avro/AvroUtil.java 1663b1d1 
  src/java/org/apache/sqoop/hive/HiveImport.java 48800366 
  src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java PRE-CREATION 
  src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java 784b5f2a 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
 2180cc20 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
 90b910a3 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
 66ebc5b8 
  src/test/org/apache/sqoop/TestParquetExport.java be1d8164 
  src/test/org/apache/sqoop/TestParquetImport.java 2810e318 
  src/test/org/apache/sqoop/TestParquetIncrementalImportMerge.java adad0cc1 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4 
  src/test/org/apache/sqoop/hive/minicluster/HiveMiniCluster.java 9dd54486 
  src/test/org/apache/sqoop/util/ParquetReader.java f1c2fe10 
  testdata/hcatalog/conf/hive-site.xml 8a84a5d3 


Diff: https://reviews.apache.org/r/68687/diff/5/

Changes: https://reviews.apache.org/r/68687/diff/4-5/


Testing
---

Ant unit and 3rd party tests were successful.
gradlew test and thirdpartytest were succesful as well.


Thanks,

Fero Szabo

Re: Review Request 68687: SQOOP-3381 Upgrade the Parquet library

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68687/
---

(Updated Oct. 15, 2018, 2:13 p.m.)


Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3381
https://issues.apache.org/jira/browse/SQOOP-3381


Repository: sqoop-trunk


Description (updated)
---

This change upgrades our parquet library to the newest version and a whole lot 
of libraries to newer versions with it.

As we will need to register a data supplier in the fix for parquet decimal 
support (SQOOP-3382), we will need a version that contains PARQUET-243. We need 
to upgrade the Parquet library to a version that contains this fix and is 
compatible with Hadoop 3.0.

A few things to note:
- hadoop's version is still 2.8.0
- hive is upgraded to 2.1.1
- the rest of the dependency changes are required for the hive version bump.

There is are a few changes in the codebase, but of course no new functionality 
at all:
- in the TestParquetImport class, the new implementation returns a Utf8 object 
for Strings written out.
- Added the security policy and related code changes from the patch for 
SQOOP-3305 (upgrade hadoop) written by Daniel Voros.
- modified HiveMiniCluster config so it won't try to start a web ui (it's 
unnecessary during tests anyway)


Diffs (updated)
-

  build.gradle fc7fc0c4 
  gradle.properties 0d30378d 
  gradle/sqoop-package.gradle 1a8d994d 
  ivy.xml 670cb32d 
  ivy/libraries.properties 8f3dab2b 
  src/java/org/apache/sqoop/avro/AvroUtil.java 1663b1d1 
  src/java/org/apache/sqoop/hive/HiveImport.java 48800366 
  src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java PRE-CREATION 
  src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java 784b5f2a 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
 2180cc20 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
 90b910a3 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
 66ebc5b8 
  src/test/org/apache/sqoop/TestParquetExport.java be1d8164 
  src/test/org/apache/sqoop/TestParquetImport.java 2810e318 
  src/test/org/apache/sqoop/TestParquetIncrementalImportMerge.java adad0cc1 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4 
  src/test/org/apache/sqoop/hive/minicluster/HiveMiniCluster.java 9dd54486 
  src/test/org/apache/sqoop/util/ParquetReader.java f1c2fe10 
  testdata/hcatalog/conf/hive-site.xml 8a84a5d3 


Diff: https://reviews.apache.org/r/68687/diff/4/

Changes: https://reviews.apache.org/r/68687/diff/3-4/


Testing (updated)
---

Ant unit and 3rd party tests were successful.
gradlew test and thirdpartytest were succesful as well.


Thanks,

Fero Szabo

Re: Review Request 68979: SQOOP-3384: Document import into external Hive table backed by S3

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68979/#review209540
---


Ship it!




Ship It!

- Fero Szabo


On Oct. 10, 2018, 3:42 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68979/
> ---
> 
> (Updated Oct. 10, 2018, 3:42 p.m.)
> 
> 
> Review request for Sqoop, Fero Szabo and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3384
> https://issues.apache.org/jira/browse/SQOOP-3384
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Document import into external Hive table backed by S3
> 
> 
> Diffs
> -
> 
>   src/docs/user/s3.txt 3724454d7efda6b390a5984d9be44d20c404f766 
> 
> 
> Diff: https://reviews.apache.org/r/68979/diff/1/
> 
> 
> Testing
> ---
> 
> ant clean docs
> ./gradlew docs
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 68712: SQOOP-3376: Test import into external Hive table backed by S3

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68712/#review209226
---


Ship it!




Lgtm!

It is interesting to see that you ran into the problem that parameterized tests 
don't support multiple dimensions!

In any case, I like the tests as they are now, they are concise enough and 
descriptive enough.

My only concern is documentation, and that it should also cover the kinks and 
quirks. But I see you've filed a separate Jira for that.


src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java
Lines 100 (patched)


Typo :) e missing from TestCase



src/test/org/apache/sqoop/testutil/S3TestUtils.java
Lines 136-138 (original), 153-155 (patched)


This sounds like something we should also mention in the user guide!


- Fero Szabo


On Sept. 24, 2018, 11:12 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68712/
> ---
> 
> (Updated Sept. 24, 2018, 11:12 p.m.)
> 
> 
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3376
> https://issues.apache.org/jira/browse/SQOOP-3376
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Testing the Text and Parquet imports into an external Hive table backed by S3.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/s3/TestS3ExternalHiveTableImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java 
> 799370816cccda7578d7c64add6e283d3123e1c8 
>   src/test/org/apache/sqoop/testutil/S3TestUtils.java 
> 0e6ef5bf001797aa70a7ad50d261c6fd384222fe 
> 
> 
> Diff: https://reviews.apache.org/r/68712/diff/3/
> 
> 
> Testing
> ---
> 
> ./gradlew test -Ds3.bucket.url= 
> -Ds3.generator.command=
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 68687: SQOOP-3381 Upgrade the Parquet library

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68687/
---

(Updated Sept. 12, 2018, 2:53 p.m.)


Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3381
https://issues.apache.org/jira/browse/SQOOP-3381


Repository: sqoop-trunk


Description
---

This change upgrades our parquet library to the newest version.

As we will need to register a data supplier in the fix for parquet decimal 
support (SQOOP-3382), we will need a version that contains PARQUET-243. We need 
to upgrade the Parquet library to a version that contains this fix and is 
compatible with Hadoop 3.0.

The only difference is found in the TestParquetImport class, the new 
implementation returns a Utf8 object for Strings written out.


Diffs
-

  gradle.properties 0d30378d 
  gradle/sqoop-package.gradle 1a8d994d 
  ivy.xml 670cb32d 
  ivy/libraries.properties 8f3dab2b 
  src/java/org/apache/sqoop/avro/AvroUtil.java 1663b1d1 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
 2180cc20 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
 90b910a3 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
 66ebc5b8 
  src/test/org/apache/sqoop/TestParquetExport.java be1d8164 
  src/test/org/apache/sqoop/TestParquetImport.java 2810e318 
  src/test/org/apache/sqoop/TestParquetIncrementalImportMerge.java adad0cc1 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4 
  src/test/org/apache/sqoop/util/ParquetReader.java f1c2fe10 


Diff: https://reviews.apache.org/r/68687/diff/3/


Testing (updated)
---

Ant unit and 3rd party tests were successful.
gradlew test was successful
gradlew thirdPartyTest was successful as well. (it had 1 failed test at first 
run: HBaseImportTest#testAppendWithTimestampSucceeds)


Thanks,

Fero Szabo

Re: Review Request 68687: SQOOP-3381 Upgrade the Parquet library

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68687/
---

(Updated Sept. 12, 2018, 2:15 p.m.)


Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3381
https://issues.apache.org/jira/browse/SQOOP-3381


Repository: sqoop-trunk


Description
---

This change upgrades our parquet library to the newest version.

As we will need to register a data supplier in the fix for parquet decimal 
support (SQOOP-3382), we will need a version that contains PARQUET-243. We need 
to upgrade the Parquet library to a version that contains this fix and is 
compatible with Hadoop 3.0.

The only difference is found in the TestParquetImport class, the new 
implementation returns a Utf8 object for Strings written out.


Diffs
-

  gradle.properties 0d30378d 
  gradle/sqoop-package.gradle 1a8d994d 
  ivy.xml 670cb32d 
  ivy/libraries.properties 8f3dab2b 
  src/java/org/apache/sqoop/avro/AvroUtil.java 1663b1d1 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
 2180cc20 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
 90b910a3 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
 66ebc5b8 
  src/test/org/apache/sqoop/TestParquetExport.java be1d8164 
  src/test/org/apache/sqoop/TestParquetImport.java 2810e318 
  src/test/org/apache/sqoop/TestParquetIncrementalImportMerge.java adad0cc1 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4 
  src/test/org/apache/sqoop/util/ParquetReader.java f1c2fe10 


Diff: https://reviews.apache.org/r/68687/diff/3/


Testing (updated)
---

Ant unit and 3rd party tests were successful.
gradlew test was successful
gradlew thirdPartyTest had 1 failed test: 
HBaseImportTest#testAppendWithTimestampSucceeds

This seems to be unrealted.


Thanks,

Fero Szabo

Review Request 68687: SQOOP-3381 Upgrade the Parquet library

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68687/
---

Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3381
https://issues.apache.org/jira/browse/SQOOP-3381


Repository: sqoop-trunk


Description
---

This change upgrades our parquet library to the newest version.

As we will need to register a data supplier in the fix for parquet decimal 
support (SQOOP-3382), we will need a version that contains PARQUET-243. We need 
to upgrade the Parquet library to a version that contains this fix and is 
compatible with Hadoop 3.0.

The only difference is found in the TestParquetImport class, the new 
implementation returns a Utf8 object for Strings written out.


Diffs
-

  gradle.properties 0d30378d 
  gradle/sqoop-package.gradle 1a8d994d 
  ivy.xml 670cb32d 
  ivy/libraries.properties 8f3dab2b 
  src/java/org/apache/sqoop/avro/AvroUtil.java 1663b1d1 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
 2180cc20 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
 90b910a3 
  
src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
 66ebc5b8 
  src/test/org/apache/sqoop/TestParquetExport.java be1d8164 
  src/test/org/apache/sqoop/TestParquetImport.java 2810e318 
  src/test/org/apache/sqoop/TestParquetIncrementalImportMerge.java adad0cc1 
  src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4 
  src/test/org/apache/sqoop/util/ParquetReader.java f1c2fe10 


Diff: https://reviews.apache.org/r/68687/diff/1/


Testing
---

Ant unit and 3rd party tests were successful.


Thanks,

Fero Szabo

Re: Review Request 68569: HiveMiniCluster does not restore hive-site.xml location

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68569/#review208248
---


Ship it!




Ship It!

- Fero Szabo


On Aug. 30, 2018, 11:27 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68569/
> ---
> 
> (Updated Aug. 30, 2018, 11:27 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3375
> https://issues.apache.org/jira/browse/SQOOP-3375
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> HiveMiniCluster sets the hive-site.xml location using 
> org.apache.hadoop.hive.conf.HiveConf#setHiveSiteLocation static method during 
> startup but it does not restore the original location during shutdown.
> 
> This makes HCatalogImportTest and HCatalogExportTest fail if they are ran in 
> the same JVM after any test using HiveMiniCluster.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/hive/minicluster/HiveMiniCluster.java 19bb7605c 
> 
> 
> Diff: https://reviews.apache.org/r/68569/diff/1/
> 
> 
> Testing
> ---
> 
> Executed unit and third party tests.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68475: SQOOP-3363: Test incremental import with S3

[Fero Szabo via Review Board]

> On Aug. 28, 2018, 11:41 a.m., Fero Szabo wrote:
> >

All ant tests passed on my system. (unit, 3rd party and s3). Gradle unit tests 
passed as well.


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68475/#review208030
---


On Aug. 28, 2018, 8:33 a.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68475/
> ---
> 
> (Updated Aug. 28, 2018, 8:33 a.m.)
> 
> 
> Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and 
> Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3363
> https://issues.apache.org/jira/browse/SQOOP-3363
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> * Added new test cases for Parquet import into S3 as it was still missing
> * Added new test cases for incremental append import into S3 in Text, Avro, 
> Sequence and Parquet file format
> * Added new test cases for incremental merge import into S3 in Text and 
> Parquet file format
> * Updated some previously added logic in S3 util and test classes
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/util/AppendUtils.java 
> 20c0d13c391d312a943a147882cf85f86cb7e438 
>   src/java/org/apache/sqoop/util/FileSystemUtil.java 
> 96ec21233d6378865195008f5ab67a74370fa2ed 
>   src/test/org/apache/sqoop/TestAppendUtils.java 
> 3d66beca0226dd9eb8a14f4157444f24a247491a 
>   src/test/org/apache/sqoop/s3/TestS3AvroImport.java 
> e130c42104b86e854d45babc009a5f1409a74a48 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendAvroImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendParquetImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendSequenceFileImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendTextImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeParquetImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeTextImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3ParquetImport.java PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3SequenceFileImport.java 
> c17c1c54918df0b4d1ecbaef4e381975d72756ae 
>   src/test/org/apache/sqoop/s3/TestS3TextImport.java 
> 60e2cd3025e67ecd43bdfb6b30d1b8d69a50da86 
>   src/test/org/apache/sqoop/testutil/AvroTestUtils.java 
> 04a8494a5d1d8a5020d5a3b629bbab62d3c09ffd 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java 
> ad2f10a071a0859c5b5e063c8cf5dda7c202124f 
>   src/test/org/apache/sqoop/testutil/S3TestUtils.java 
> ceaff3b3a2bfd031b9772c9b43afdfa670c23718 
>   src/test/org/apache/sqoop/testutil/SequenceFileTestUtils.java 
> ad7576dbb2447423c677429f24163031a9d39b5f 
>   src/test/org/apache/sqoop/testutil/TextFileTestUtils.java 
> df19cb8be7a633a6f1e1e3f9bc7d0dbc268aa90a 
> 
> 
> Diff: https://reviews.apache.org/r/68475/diff/5/
> 
> 
> Testing
> ---
> 
> ant clean test -Ds3.bucket.url= 
> -Ds3.generator.command=
> ./gradlew test -Ds3.bucket.url= 
> -Ds3.generator.command=
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 68475: SQOOP-3363: Test incremental import with S3

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68475/#review208030
---


Fix it, then Ship it!





src/test/org/apache/sqoop/testutil/S3TestUtils.java
Lines 181-185 (patched)


nit: indentation missing.


- Fero Szabo


On Aug. 28, 2018, 8:33 a.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68475/
> ---
> 
> (Updated Aug. 28, 2018, 8:33 a.m.)
> 
> 
> Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and 
> Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3363
> https://issues.apache.org/jira/browse/SQOOP-3363
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> * Added new test cases for Parquet import into S3 as it was still missing
> * Added new test cases for incremental append import into S3 in Text, Avro, 
> Sequence and Parquet file format
> * Added new test cases for incremental merge import into S3 in Text and 
> Parquet file format
> * Updated some previously added logic in S3 util and test classes
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/util/AppendUtils.java 
> 20c0d13c391d312a943a147882cf85f86cb7e438 
>   src/java/org/apache/sqoop/util/FileSystemUtil.java 
> 96ec21233d6378865195008f5ab67a74370fa2ed 
>   src/test/org/apache/sqoop/TestAppendUtils.java 
> 3d66beca0226dd9eb8a14f4157444f24a247491a 
>   src/test/org/apache/sqoop/s3/TestS3AvroImport.java 
> e130c42104b86e854d45babc009a5f1409a74a48 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendAvroImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendParquetImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendSequenceFileImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendTextImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeParquetImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeTextImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3ParquetImport.java PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3SequenceFileImport.java 
> c17c1c54918df0b4d1ecbaef4e381975d72756ae 
>   src/test/org/apache/sqoop/s3/TestS3TextImport.java 
> 60e2cd3025e67ecd43bdfb6b30d1b8d69a50da86 
>   src/test/org/apache/sqoop/testutil/AvroTestUtils.java 
> 04a8494a5d1d8a5020d5a3b629bbab62d3c09ffd 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java 
> ad2f10a071a0859c5b5e063c8cf5dda7c202124f 
>   src/test/org/apache/sqoop/testutil/S3TestUtils.java 
> ceaff3b3a2bfd031b9772c9b43afdfa670c23718 
>   src/test/org/apache/sqoop/testutil/SequenceFileTestUtils.java 
> ad7576dbb2447423c677429f24163031a9d39b5f 
>   src/test/org/apache/sqoop/testutil/TextFileTestUtils.java 
> df19cb8be7a633a6f1e1e3f9bc7d0dbc268aa90a 
> 
> 
> Diff: https://reviews.apache.org/r/68475/diff/5/
> 
> 
> Testing
> ---
> 
> ant clean test -Ds3.bucket.url= 
> -Ds3.generator.command=
> ./gradlew test -Ds3.bucket.url= 
> -Ds3.generator.command=
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 68516: Improve third party tests to be able to execute them in a single JVM

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68516/#review208019
---


Ship it!




Unit and S3 tests passed for me as well. (Except for the ones with Kerberos, of 
course.)

- Fero Szabo


On Aug. 27, 2018, 12:38 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68516/
> ---
> 
> (Updated Aug. 27, 2018, 12:38 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3367
> https://issues.apache.org/jira/browse/SQOOP-3367
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> The goal of this JIRA is to improve the third party tests to be able to 
> execute them in a single JVM. See the parent JIRA for the details.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/hive/HiveImport.java 5da00a740 
>   src/test/org/apache/sqoop/manager/oracle/OraOopTypesTest.java a8072b92a 
>   src/test/org/apache/sqoop/manager/oracle/SystemImportTest.java e0a0462c3 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java ad2f10a07 
>   src/test/org/apache/sqoop/testutil/LobAvroImportTestCase.java 20d6151ba 
>   testdata/hive/scripts/customDelimImport.q a27396947 
>   testdata/hive/scripts/dateImport.q 476d4310c 
>   testdata/hive/scripts/decimalMapImport.q 8f05d585d 
>   testdata/hive/scripts/failingImport.q 476d4310c 
>   testdata/hive/scripts/fieldWithNewlineImport.q c6c9ebdaa 
>   testdata/hive/scripts/fieldWithNewlineReplacementImport.q a4b8a5992 
>   testdata/hive/scripts/incrementalHiveAppend10.q 383c7b4aa 
>   testdata/hive/scripts/incrementalHiveAppend20.q 383c7b4aa 
>   testdata/hive/scripts/incrementalHiveAppendEmpty.q 383c7b4aa 
>   testdata/hive/scripts/normalImport.q 34d6ac1ea 
>   testdata/hive/scripts/numericImport.q bef7c715e 
>   testdata/hive/scripts/partitionImport.q 68ce7c1bb 
> 
> 
> Diff: https://reviews.apache.org/r/68516/diff/1/
> 
> 
> Testing
> ---
> 
> Ran unit and third party tests.
> Note that if you want to run the tests with forkEvery set to 0, you have to 
> disable the kereberized test cases first as I did here: 
> https://github.com/szvasas/sqoop/commit/d9ba8a511ed0bad119e105b97f4725ea0951
> Setting the forkEvery to 0: 
> https://github.com/szvasas/sqoop/commit/6332c2959bafb95e83c2d9a4030ba432cfd4c640
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68475: SQOOP-3363: Test incremental import with S3

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68475/#review207962
---




src/test/org/apache/sqoop/s3/TestS3IncrementalMergeTextImport.java
Lines 120-121 (patched)


These two constants are defined 6 times in the patch. Why not move them 
outside of these methods and make them static?



src/test/org/apache/sqoop/s3/TestS3ParquetImport.java
Lines 77 (patched)


Why is this protected (as opposed to private)?



src/test/org/apache/sqoop/s3/TestS3ParquetImport.java
Lines 85-88 (patched)


I see that there are some duplications in the code. For example, you could 
extract these 4 lines into a new method.


- Fero Szabo


On Aug. 24, 2018, 2:41 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68475/
> ---
> 
> (Updated Aug. 24, 2018, 2:41 p.m.)
> 
> 
> Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and 
> Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3363
> https://issues.apache.org/jira/browse/SQOOP-3363
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> * Added new test cases for Parquet import into S3 as it was still missing
> * Added new test cases for incremental append import into S3 in Text, Avro, 
> Sequence and Parquet file format
> * Added new test cases for incremental merge import into S3 in Text and 
> Parquet file format
> * Updated some previously added logic in S3 util and test classes
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/util/FileSystemUtil.java 
> 96ec21233d6378865195008f5ab67a74370fa2ed 
>   src/test/org/apache/sqoop/s3/TestS3AvroImport.java 
> e130c42104b86e854d45babc009a5f1409a74a48 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendAvroImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendParquetImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendSequenceFileImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalAppendTextImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeParquetImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3IncrementalMergeTextImport.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3ParquetImport.java PRE-CREATION 
>   src/test/org/apache/sqoop/s3/TestS3SequenceFileImport.java 
> c17c1c54918df0b4d1ecbaef4e381975d72756ae 
>   src/test/org/apache/sqoop/s3/TestS3TextImport.java 
> 60e2cd3025e67ecd43bdfb6b30d1b8d69a50da86 
>   src/test/org/apache/sqoop/testutil/AvroTestUtils.java 
> 04a8494a5d1d8a5020d5a3b629bbab62d3c09ffd 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java 
> 1730698e80cc77395f8a296b7bf01c104533e10b 
>   src/test/org/apache/sqoop/testutil/S3TestUtils.java 
> ceaff3b3a2bfd031b9772c9b43afdfa670c23718 
>   src/test/org/apache/sqoop/testutil/SequenceFileTestUtils.java 
> ad7576dbb2447423c677429f24163031a9d39b5f 
>   src/test/org/apache/sqoop/testutil/TextFileTestUtils.java 
> df19cb8be7a633a6f1e1e3f9bc7d0dbc268aa90a 
> 
> 
> Diff: https://reviews.apache.org/r/68475/diff/4/
> 
> 
> Testing
> ---
> 
> ant clean test -Ds3.bucket.url= 
> -Ds3.generator.command=
> ./gradlew test -Ds3.bucket.url= 
> -Ds3.generator.command=
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 68500: Fix tests using HiveMiniCluster

[Fero Szabo via Review Board]

> On Aug. 24, 2018, 1:35 p.m., Fero Szabo wrote:
> > Hi Szabi,
> > 
> > I tried to reproduce the issue without your change, by running 
> > org.apache.sqoop.hive.TestHiveServer2TextImport, but couldn't.
> > 
> > I deleted calcite from the ~/.ivy2 and ~/m2 directories and calcite was 
> > downloaded.
> > 
> > Am I missing something? (I haven't run the test with gradle, yet, is it 
> > related to only gradle maybe?)
> > 
> > The command I used was this: 
> > ant clean test -Dtestcase=TestHiveServer2TextImport
> 
> Szabolcs Vasas wrote:
> No, it is indeed very tricky, I have not experienced it for a long time 
> but then the issue suddenly appeared and I constantly get it without this 
> change since then. I have also been able to reproduce it on an Ubuntu VM.

As discussed offline, let's commit this change!


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68500/#review207877
---


On Aug. 24, 2018, 1:16 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68500/
> ---
> 
> (Updated Aug. 24, 2018, 1:16 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3371
> https://issues.apache.org/jira/browse/SQOOP-3371
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> It seems that SQOOP-3360 broke our tests which use HiveMiniCluster because 
> org.apache.calcite is not present in the hive-exec:core JAR but this 
> dependency seems to be needed by these tests.
> 
> I am not sure why our Jenkins job did not catch the issue earlier but I get 
> consistent failures when I run these tests with a clean ivy cache and ant so 
> the dependency issue needs to be fixed.
> 
> 
> Diffs
> -
> 
>   build.gradle 9a2b55cfc 
>   gradle.properties a71729e12 
>   ivy.xml 184486029 
>   ivy/libraries.properties 2f1ec9e50 
> 
> 
> Diff: https://reviews.apache.org/r/68500/diff/1/
> 
> 
> Testing
> ---
> 
> Executed unit and third party tests with both ant and gradle.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68500: Fix tests using HiveMiniCluster

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68500/#review207950
---


Ship it!




Ship It!

- Fero Szabo


On Aug. 24, 2018, 1:16 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68500/
> ---
> 
> (Updated Aug. 24, 2018, 1:16 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3371
> https://issues.apache.org/jira/browse/SQOOP-3371
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> It seems that SQOOP-3360 broke our tests which use HiveMiniCluster because 
> org.apache.calcite is not present in the hive-exec:core JAR but this 
> dependency seems to be needed by these tests.
> 
> I am not sure why our Jenkins job did not catch the issue earlier but I get 
> consistent failures when I run these tests with a clean ivy cache and ant so 
> the dependency issue needs to be fixed.
> 
> 
> Diffs
> -
> 
>   build.gradle 9a2b55cfc 
>   gradle.properties a71729e12 
>   ivy.xml 184486029 
>   ivy/libraries.properties 2f1ec9e50 
> 
> 
> Diff: https://reviews.apache.org/r/68500/diff/1/
> 
> 
> Testing
> ---
> 
> Executed unit and third party tests with both ant and gradle.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68500: Fix tests using HiveMiniCluster

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68500/#review207877
---



Hi Szabi,

I tried to reproduce the issue without your change, by running 
org.apache.sqoop.hive.TestHiveServer2TextImport, but couldn't.

I deleted calcite from the ~/.ivy2 and ~/m2 directories and calcite was 
downloaded.

Am I missing something? (I haven't run the test with gradle, yet, is it related 
to only gradle maybe?)

The command I used was this: 
ant clean test -Dtestcase=TestHiveServer2TextImport

- Fero Szabo


On Aug. 24, 2018, 1:16 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68500/
> ---
> 
> (Updated Aug. 24, 2018, 1:16 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3371
> https://issues.apache.org/jira/browse/SQOOP-3371
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> It seems that SQOOP-3360 broke our tests which use HiveMiniCluster because 
> org.apache.calcite is not present in the hive-exec:core JAR but this 
> dependency seems to be needed by these tests.
> 
> I am not sure why our Jenkins job did not catch the issue earlier but I get 
> consistent failures when I run these tests with a clean ivy cache and ant so 
> the dependency issue needs to be fixed.
> 
> 
> Diffs
> -
> 
>   build.gradle 9a2b55cfc 
>   gradle.properties a71729e12 
>   ivy.xml 184486029 
>   ivy/libraries.properties 2f1ec9e50 
> 
> 
> Diff: https://reviews.apache.org/r/68500/diff/1/
> 
> 
> Testing
> ---
> 
> Executed unit and third party tests with both ant and gradle.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68470: SQOOP-3366 Improve unit tests to be able to execute them in a single JVM

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68470/#review207820
---


Ship it!




Ship It!

- Fero Szabo


On Aug. 23, 2018, 3:54 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68470/
> ---
> 
> (Updated Aug. 23, 2018, 3:54 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3366
> https://issues.apache.org/jira/browse/SQOOP-3366
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Fix the unit tests which changed the state of the JVM so that other tests 
> will not be affected when they are run on a single JVM.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/TestIncrementalImport.java e1faf351f 
>   src/test/org/apache/sqoop/TestSqoopOptions.java d0591ad0d 
>   
> src/test/org/apache/sqoop/metastore/TestMetastoreConfigurationParameters.java 
> 0f1eb890b 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java 1730698e8 
>   src/test/org/apache/sqoop/testutil/HsqldbTestServer.java c63a8f2dc 
>   src/test/org/apache/sqoop/tool/TestMainframeImportTool.java c2edc5347 
> 
> 
> Diff: https://reviews.apache.org/r/68470/diff/3/
> 
> 
> Testing
> ---
> 
> No test has been added.
> 
> 
> Thanks,
> 
> Nguyen Truong
> 
>

Re: Review Request 37353: Support snappy compression in Sqoop Import with HCatalog.The Jira is SQOOP-2331

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37353/#review207078
---



Hi Shashank,

Other than the minor issue with the default value of the compression codec, 
this change looks ok to me. However, since these are new features in Sqoop, can 
you please add a few tests for them?

You can find similar tests to what you'll need in HCatalogImportTest.

The best way would be to add a new test class (it could be called 
HCatalogImportWithCompressionTest) and also utilize the ArgumentArrayBuilder 
class. You can find an example on how to use the builder in one of the more 
recent tests, such as the newly added S3 tests (TestS3AvroImport.java which 
calls into S3TestUtils.java). Anyway, please make sure to test both file 
formats. If you can think of multiple testcases that make sense, that's also 
welcome! 

I will be on vacation till the August 22, but hopefully other members of the 
community will help you out if you've any questions.

(Just for the record, I've also ran the unit and 3rd party tests, and didn't 
find any issues.)


src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java
Lines 900-904 (patched)


The default codec is gzip. However, if the codecName is not specified by 
the user, execution will never reach this point in the code. Please revise.



src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java
Lines 910 (patched)


nit: missing space before else



src/java/org/apache/sqoop/tool/ImportTool.java
Lines 1178-1179 (patched)


I think there is a mismatch between the validation and the setup logic. 

Here in the validation there is no default value, but an exception is 
thrown. In the setup logic, if codec is not specified GZIP is used as a 
default. 

I believe if the codec is not set, we should log a warning, and tell the 
user that the default will be used i.e. GZIP (or change it to SNAPPY if that's 
preferable).


- Fero Szabo


On Aug. 10, 2018, 7:29 a.m., Shashank Tandon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37353/
> ---
> 
> (Updated Aug. 10, 2018, 7:29 a.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed, Fero Szabo, Szabolcs Vasas, and 
> Venkat Ranganathan.
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Apache Sqoop does not compress with --compress option with 
> --hcatalog-table.It also does not support option --compression-codec snappy. 
> Will add Snappy compression support in Apache Sqoop. When a user will try to 
> use --compress, then it will use the by default compression i.e. GZIP. 
> otherwise If user provide option --compress --compression-codec snappy then 
> it will compress into snappy format.
> 
> 
> Diffs
> -
> 
>   src/docs/user/hcatalog.txt 2ae1d54d 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 3724f250 
>   src/java/org/apache/sqoop/io/CodecMap.java d5796188 
>   src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java 784b5f2a 
>   src/java/org/apache/sqoop/tool/ImportTool.java ccded652 
>   src/test/org/apache/sqoop/hcat/TestHCatalogBasic.java ba05cabb 
> 
> 
> Diff: https://reviews.apache.org/r/37353/diff/3/
> 
> 
> Testing
> ---
> 
> yes
> 
> 
> File Attachments
> 
> 
> SQOOP-2331_2.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/07/24/d474a04e-fe57-4c06-a066-0b70befcd29d__SQOOP-2331_2.patch
> SQOOP-2331_2.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/08/10/c4004353-2230-4ef9-9ccc-c4934314f548__SQOOP-2331_3.patch
> SQOOP-2331_3.patch
>   
> https://reviews.apache.org/media/uploaded/files/2018/08/10/bf01aa4e-b9d2-43ac-bb5a-80a4787ad1a5__SQOOP-2331_3.patch
> 
> 
> Thanks,
> 
> Shashank Tandon
> 
>

Re: Review Request 68278: Add LOG message for git hash

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68278/#review207025
---




src/java/org/apache/sqoop/Sqoop.java
Lines 95 (patched)


You could extract SqoopVersion into a local variable and reuse it.


- Fero Szabo


On Aug. 9, 2018, 1:32 p.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68278/
> ---
> 
> (Updated Aug. 9, 2018, 1:32 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3359
> https://issues.apache.org/jira/browse/SQOOP-3359
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> I added a log message for the git hash of the running Sqoop in the debug 
> level.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/Sqoop.java 
> 08ff82ca22cb2fcdffeb29661951d091a70c5bb9 
> 
> 
> Diff: https://reviews.apache.org/r/68278/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Nguyen Truong
> 
>

Re: Review Request 68254: Change MainframeImportTool to refer to MainframeManager class directly

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68254/#review207024
---


Ship it!




Ship It!

- Fero Szabo


On Aug. 7, 2018, 9:33 a.m., Nguyen Truong wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68254/
> ---
> 
> (Updated Aug. 7, 2018, 9:33 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3357
> https://issues.apache.org/jira/browse/SQOOP-3357
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> MainframeImportTool used to refer to the MainframeManager with a string. I 
> changed the string to a MainframeManager class reference.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/tool/MainframeImportTool.java 8883301d 
>   src/test/org/apache/sqoop/manager/TestMainframeManager.java 97e48e88 
> 
> 
> Diff: https://reviews.apache.org/r/68254/diff/1/
> 
> 
> Testing
> ---
> 
> I ran ant test and did not add any more test case because my change was 
> already covered.
> 
> 
> Thanks,
> 
> Nguyen Truong
> 
>

Re: Review Request 68023: Remove Kite dependency from the Gradle dependencies

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68023/#review206542
---


Ship it!




Lgtm. Unit and 3rd party are successful.

- Fero Szabo


On July 27, 2018, 10:05 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68023/
> ---
> 
> (Updated July 27, 2018, 10:05 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3349
> https://issues.apache.org/jira/browse/SQOOP-3349
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Removed Kite from the Gradle dependencies, cleaned up some config files 
> containing references to Kite.
> 
> 
> Diffs
> -
> 
>   LICENSE.txt 48b2c3bc4 
>   gradle.properties 07224905a 
>   gradle/sqoop-package.gradle 076fed858 
>   testdata/hcatalog/conf/hive-site.xml 4922abd57 
> 
> 
> Diff: https://reviews.apache.org/r/68023/diff/1/
> 
> 
> Testing
> ---
> 
> Unit and third party tests.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68038: Sqoop should not check incremental constraints for HBase imports

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68038/#review206537
---


Ship it!




Ship It!

- Fero Szabo


On July 25, 2018, 2:44 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68038/
> ---
> 
> (Updated July 25, 2018, 2:44 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3353
> https://issues.apache.org/jira/browse/SQOOP-3353
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> See the corresponding JIRA for the description of the bug.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/tool/ImportTool.java ccded652c 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 1ab98021a 
>   src/test/org/apache/sqoop/tool/TestImportTool.java 3bdc5c654 
> 
> 
> Diff: https://reviews.apache.org/r/68038/diff/1/
> 
> 
> Testing
> ---
> 
> Executed test and third party tests using ant.
> Gradle test is still unstable because of 
> https://issues.apache.org/jira/browse/SQOOP-3350
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 68045: Fix tests which use warehouse-dir as target-dir

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68045/#review206506
---


Ship it!




Applies cleanly and tests are successful on my side.

- Fero Szabo


On July 25, 2018, 2:51 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68045/
> ---
> 
> (Updated July 25, 2018, 2:51 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3350
> https://issues.apache.org/jira/browse/SQOOP-3350
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> See JIRA description
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/TestFreeFormQueryImport.java 2df4352a0 
>   src/test/org/apache/sqoop/manager/mysql/MySqlColumnEscapeImportTest.java 
> 734499e58 
>   src/test/org/apache/sqoop/manager/oracle/OracleColumnEscapeImportTest.java 
> d4146dcbe 
>   src/test/org/apache/sqoop/manager/oracle/OracleIncrementalImportTest.java 
> 4dde3d62f 
>   
> src/test/org/apache/sqoop/manager/oracle/OracleSpecialCharacterTableImportTest.java
>  453ad8218 
>   src/test/org/apache/sqoop/manager/oracle/OracleSplitterTest.java 33b7d7452 
> 
> 
> Diff: https://reviews.apache.org/r/68045/diff/1/
> 
> 
> Testing
> ---
> 
> Executed unit and third party tests.
> Having SQOOP-3353 committed all tests should pass with Gradle too.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Review Request 68064: SQOOP-3355 Document SQOOP-1905 DB2 --schema option

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68064/
---

Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3355
https://issues.apache.org/jira/browse/SQOOP-3355


Repository: sqoop-trunk


Description
---

Adding documentation for a previously implemented feature. This one is quite 
simple.


Diffs
-

  src/docs/user/connectors.txt 59e3e00b 


Diff: https://reviews.apache.org/r/68064/diff/1/


Testing
---

ant docs, 
+ unit and 3rd party tests, though these shouldn't be affected.


Thanks,

Fero Szabo

Re: Review Request 68032: SQOOP-3352: Bump java target version to 1.8

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68032/#review206442
---


Ship it!




Ship It!

- Fero Szabo


On July 25, 2018, 9:43 a.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68032/
> ---
> 
> (Updated July 25, 2018, 9:43 a.m.)
> 
> 
> Review request for Sqoop, Fero Szabo and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3352
> https://issues.apache.org/jira/browse/SQOOP-3352
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> * Bumped targetJavaVersion and sourceJavaVersion to 1.8 in build.xml
> * Bumped javaSourceCompatibilityVersion to 1.8 in gradle.properties
> * Updated documentation
> 
> 
> Diffs
> -
> 
>   COMPILING.txt 9abf0b211b24de65b53351ece170b09e4851938b 
>   README.txt b8f0a7a2c7d760c72f9811435b36c23cde354b15 
>   build.gradle 8b8b55575ab05187eec0b8ca6b6da17e0a318d72 
>   build.xml 0ae729bce035f6f1afef78702a3d888bb6c5a9ff 
>   gradle.properties a571e6cb26ddd3d687f3b3a2a69615a5e1fc71d5 
>   src/docs/man/sqoop.txt 7977e13091af184f6b1d10028aa827e033c9452a 
> 
> 
> Diff: https://reviews.apache.org/r/68032/diff/3/
> 
> 
> Testing
> ---
> 
> Successfully ran with ant:
> ant
> ant clean compile
> ant clean package
> ant clean test
> ant cobertura -Dcobertura.home=/path/to/cobertura
> ant clean test -Dthirdparty= ...
> ant checkstyle
> ant mvn-install
> ant docs
> ant eclipse
> 
> Succesfully ran with gradle:
> ./gradlew clean
> ./gradlew package
> ./gradlew test
> ./gradlew jacocoTestReport
> ./gradlew -Dsqoop.thirdparty.lib.dir= 
> thirdPartyTest
> ./gradlew checkStyleMain
> ./gradlew compileJava
> ./gradlew publishToMavenLocal
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 68000: SQOOP-3347: Make verify() more generic in AvroTestUtils

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68000/#review206281
---


Ship it!




Lgtm!

My 50cents: Seems like, in the long term we might end up refactoring this class 
to the factory pattern.

Cheers,
Fero

- Fero Szabo


On July 20, 2018, 1:47 p.m., Boglarka Egyed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68000/
> ---
> 
> (Updated July 20, 2018, 1:47 p.m.)
> 
> 
> Review request for Sqoop, daniel voros, Fero Szabo, and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3347
> https://issues.apache.org/jira/browse/SQOOP-3347
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Extracting decimal conversion logic from verify() method of AvroTestUtils 
> class.
> 
> 
> Diffs
> -
> 
>   src/test/org/apache/sqoop/importjob/avro/AvroImportForNumericTypesTest.java 
> e04caf7c7c1fba24a47858c609a00ecac9f71cc5 
>   src/test/org/apache/sqoop/manager/hsqldb/TestHsqldbAvroPadding.java 
> 7e42bf1e3242acc26ae196986daa96a9160196ed 
>   src/test/org/apache/sqoop/testutil/AvroTestUtils.java 
> 75940bf1fe77ba4655d88c3c0f415c548201daf9 
> 
> 
> Diff: https://reviews.apache.org/r/68000/diff/1/
> 
> 
> Testing
> ---
> 
> Ran unit and 3rd party tests successfully.
> 
> 
> Thanks,
> 
> Boglarka Egyed
> 
>

Re: Review Request 67929: Remove Kite dependency from the Sqoop project

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67929/#review206150
---


Ship it!




A long-awaited patch! :)

(Anyway, you could link the review to the Jira as well.)

- Fero Szabo


On July 16, 2018, 3:56 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67929/
> ---
> 
> (Updated July 16, 2018, 3:56 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3329
> https://issues.apache.org/jira/browse/SQOOP-3329
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> - Removed kitesdk dependency from ivy.xml
> - Removed Kite Dataset API based Parquet import implementation
> - Since Parquet library was a transitive dependency of the Kite SDK I added 
> org.apache.parquet.avro-parquet 1.9 as a direct dependency
> - In this dependency the parquet package has changed to org.apache.parquet so 
> I needed to make changes in several classes according to this
> - Removed all the Parquet related test cases from TestHiveImport. These 
> scenarios are already covered in TestHiveServer2ParquetImport.
> - Modified the documentation to reflect these changes.
> 
> 
> Diffs
> -
> 
>   ivy.xml 1f587f3eb 
>   ivy/libraries.properties 565a8bf50 
>   src/docs/user/hive-notes.txt af97d94b3 
>   src/docs/user/import.txt a2c16d956 
>   src/java/org/apache/sqoop/SqoopOptions.java cc1b75281 
>   src/java/org/apache/sqoop/avro/AvroUtil.java 1663b1d1a 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorImplementation.java
>  050c85488 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
>  2180cc20e 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
>  90b910a34 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
>  66ebc5b80 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteMergeParquetReducer.java 
> 02816d77f 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetExportJobConfigurator.java
>  6ebc5a31b 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetExportMapper.java 
> 122ff3fc9 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetImportJobConfigurator.java
>  7e179a27d 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetImportMapper.java 
> 0a91e4a20 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetJobConfiguratorFactory.java
>  bd07c09f4 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetMergeJobConfigurator.java
>  ed045cd14 
>   src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetUtils.java 
> a4768c932 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 87fc5e987 
>   src/test/org/apache/sqoop/TestMerge.java 2b3280a5a 
>   src/test/org/apache/sqoop/TestParquetExport.java 0fab1880c 
>   src/test/org/apache/sqoop/TestParquetImport.java b1488e8af 
>   src/test/org/apache/sqoop/TestParquetIncrementalImportMerge.java adad0cc11 
>   src/test/org/apache/sqoop/hive/TestHiveImport.java 436f0e512 
>   src/test/org/apache/sqoop/hive/TestHiveServer2ParquetImport.java b55179a4f 
>   src/test/org/apache/sqoop/tool/TestBaseSqoopTool.java dbda8b7f4 
>   src/test/org/apache/sqoop/util/ParquetReader.java f1c2fe10a 
> 
> 
> Diff: https://reviews.apache.org/r/67929/diff/1/
> 
> 
> Testing
> ---
> 
> Ran unit and third party tests.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 67926: Document Parquet support

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67926/#review206101
---


Ship it!




Ship It!

- Fero Szabo


On July 16, 2018, 1:42 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67926/
> ---
> 
> (Updated July 16, 2018, 1:42 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3338
> https://issues.apache.org/jira/browse/SQOOP-3338
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Documenting Parquet support and covering the changes brought in by SQOOP-3313.
> 
> 
> Diffs
> -
> 
>   src/docs/user/hive-args.txt 8af9a1c51 
>   src/docs/user/hive-notes.txt deee27021 
>   src/docs/user/import.txt 2d074f492 
> 
> 
> Diff: https://reviews.apache.org/r/67926/diff/2/
> 
> 
> Testing
> ---
> 
> ant docs is executed successfully.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 67926: Document Parquet support

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67926/#review206097
---



LGTM.

Perhaps you could consider another example with the compression codecs 
specified. Is my understanding correct, that the compression codec has to come 
from an option, and not from a system property or the site.xml?


src/docs/user/import.txt
Lines 481 (patched)


Perhaps it would make sense to give an example with a hs2 url and a 
compression codec.


- Fero Szabo


On July 16, 2018, 12:15 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67926/
> ---
> 
> (Updated July 16, 2018, 12:15 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3338
> https://issues.apache.org/jira/browse/SQOOP-3338
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Documenting Parquet support and covering the changes brought in by SQOOP-3313.
> 
> 
> Diffs
> -
> 
>   src/docs/user/hive-args.txt 8af9a1c51 
>   src/docs/user/hive-notes.txt deee27021 
>   src/docs/user/import.txt 2d074f492 
> 
> 
> Diff: https://reviews.apache.org/r/67926/diff/1/
> 
> 
> Testing
> ---
> 
> ant docs is executed successfully.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 67675: SQOOP-3332 Extend Documentation of --resilient flag and add warning message when detected

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67675/
---

(Updated June 28, 2018, 12:29 p.m.)


Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Changes
---

small correction to make the implicit requirement a little bit clearer.


Bugs: SQOOP-3332
https://issues.apache.org/jira/browse/SQOOP-3332


Repository: sqoop-trunk


Description
---

This is the documentation part of SQOOP-.


Diffs (updated)
-

  src/docs/user/connectors.txt f1c7aebe 
  src/java/org/apache/sqoop/manager/SQLServerManager.java c98ad2db 
  src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
cf58f631 
  src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
fc1c4895 


Diff: https://reviews.apache.org/r/67675/diff/3/

Changes: https://reviews.apache.org/r/67675/diff/2-3/


Testing
---

Unit tests, 3rdparty tests, ant docs.

I've also investigated how export and import works: 

Import has it's retry mechanism in 
org.apache.sqoop.mapreduce.db.SQLServerDBRecordReader#nextKeyValue
In case of error, it re-calculates the db query, thus the implicit requirements

Export has it's retry loop in 
org.apache.sqoop.mapreduce.SQLServerAsyncDBExecThread#write
It doesn't recalculate the query, thus is a lot safer.


Thanks,

Fero Szabo

Re: Review Request 67675: SQOOP-3332 Extend Documentation of --resilient flag and add warning message when detected

[Fero Szabo via Review Board]

> On June 28, 2018, 8:42 a.m., daniel voros wrote:
> > Hi Fero,
> > 
> > If I understand correclty, with this patch we're only displaying a warning 
> > when using --resilient to let the users know they should add --split-by 
> > (even if they do so?).
> > 
> > In the documentation you're saying omitting --split-by can lead to 
> > lost/duplicated records. Shouldn't we stop the importing if there's no 
> > --split-by then? I understand we can't enforce the uniqeness and ascending 
> > order though, so keeping some kind of warning could make sense too.
> > 
> > What do you think?
> > 
> > Regards,
> > Daniel

Hi Dani,

Thank you for the review!

Yes, so, the warning message is always the same, though I wanted to put the 
emphasis on the implicit requirements of import (unique and ascending values in 
the split-by column). Happy to change the message if you've a better suggestion!

I haven't written anything about omitting --split-by, at least it wasn't my 
intention to. The first sentence (that I added) says:
"In case of import however, one has to use both the +--resilient+ option and 
specify the +--split-by+ column to trigger the retry mechanism."

Import doesn't use resilient operations if there is no --split-by option. 
Though I believe that it falls back to non-resilient (default) behavior.

So, what I intended to say was the same as what you're suggesting here, but it 
might be confusing, then. Do you think I should change anything?


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67675/#review205494
---


On June 25, 2018, 3:17 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67675/
> ---
> 
> (Updated June 25, 2018, 3:17 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3332
> https://issues.apache.org/jira/browse/SQOOP-3332
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This is the documentation part of SQOOP-.
> 
> 
> Diffs
> -
> 
>   src/docs/user/connectors.txt f1c7aebe 
>   src/java/org/apache/sqoop/manager/SQLServerManager.java c98ad2db 
>   src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
> cf58f631 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
> fc1c4895 
> 
> 
> Diff: https://reviews.apache.org/r/67675/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests, 3rdparty tests, ant docs.
> 
> I've also investigated how export and import works: 
> 
> Import has it's retry mechanism in 
> org.apache.sqoop.mapreduce.db.SQLServerDBRecordReader#nextKeyValue
> In case of error, it re-calculates the db query, thus the implicit 
> requirements
> 
> Export has it's retry loop in 
> org.apache.sqoop.mapreduce.SQLServerAsyncDBExecThread#write
> It doesn't recalculate the query, thus is a lot safer.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 67628: Implement an alternative solution for Parquet reading and writing

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67628/#review205359
---


Ship it!




Ship It!

- Fero Szabo


On June 26, 2018, 9:15 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67628/
> ---
> 
> (Updated June 26, 2018, 9:15 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3328
> https://issues.apache.org/jira/browse/SQOOP-3328
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> The new implementation uses classes from parquet.hadoop packages.
> TestParquetIncrementalImportMerge has been introduced to cover some gaps we 
> had in the Parquet merge support.
> The test infrastructure is also modified a bit which was needed because of 
> TestParquetIncrementalImportMerge.
> 
> Note that this JIRA does not cover the Hive Parquet import support I will 
> create another JIRA for that.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/SqoopOptions.java 
> d9984af369f901c782b1a74294291819e7d13cdd 
>   src/java/org/apache/sqoop/avro/AvroUtil.java 
> 57c2062568778c5bb53cd4118ce4f030e4ff33f2 
>   src/java/org/apache/sqoop/manager/ConnManager.java 
> c80dd5d9cbaa9b114c12b693e9a686d2cbbe51a3 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 
> 3b5421028d3006e790ed4b711a06dbdb4035b8a0 
>   src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 
> 17c9ed39b1e613a6df36b54cd5395b80e5f8fb0b 
>   src/java/org/apache/sqoop/mapreduce/parquet/ParquetConstants.java 
> ae53a96bddc523a52384715dd97705dc3d9db607 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetExportJobConfigurator.java 
> 8d7b87f6d6832ce8d81d995af4c4bd5eeae38e1b 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetImportJobConfigurator.java 
> fa1bc7d1395fbbbceb3cb72802675aebfdb27898 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorFactory.java
>  ed5103f1d84540ef2fa5de60599e94aa69156abe 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorFactoryProvider.java
>  2286a52030778925349ebb32c165ac062679ff71 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorImplementation.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetMergeJobConfigurator.java 
> 67fdf6602bcbc6c091e1e9bf4176e56658ce5222 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportMapper.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportMapper.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetJobConfiguratorFactory.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteMergeParquetReducer.java 
> 7f21205e1c4be4200f7248d3f1c8513e0c8e490c 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetExportJobConfigurator.java
>  ca02c7bdcaf2fa981e15a6a96b111dec38ba2b25 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetExportMapper.java 
> 2d88a9c8ea4eb32001e1eb03e636d9386719 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetImportJobConfigurator.java
>  87828d1413eb71761aed44ad3b138535692f9c97 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetImportMapper.java 
> 20adf6e422cc4b661a74c8def114d44a14787fc6 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetJobConfiguratorFactory.java
>  055e1166b07aeef711cd162052791500368c628d 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetMergeJobConfigurator.java
>  9fecf282885f7aeac011a66f7d5d05512624976f 
>   src/java/org/apache/sqoop/mapreduce/parquet/kite/KiteParquetUtils.java 
> e68bba90d8b08ac3978fcc9ccae612bdf02388e8 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 
> c62ee98c2b22d819c9a994884b254f76eb518b6a 
>   src/java/org/apache/sqoop/tool/ImportTool.java 
> 2c474b7eeeff02b59204e4baca8554d668b6c61e 
>   src/java/org/apache/sqoop/tool/MergeTool.java 
> 4c20f7d151514b26a098dafdc1ee265cbde5ad20 
>   src/test/org/apache/sqoop/TestBigDecimalExport.java 
> ccea17345c0c8a2bdb7c8fd141f37e3c822ee41e 
>   src/test/org/apache/sqoop/TestMerge.java 
> 11806fea6c59ea897bc1aa23f6657ed172d093d5 
>   src/test/org/apache/sqoop/TestParquetExport.java 
> 43dabb57b7862b607490369e09b197b6de65a147 
>   src/test/org/apache/sqoop/TestParquetImport.java 
> 

Re: Review Request 67675: SQOOP-3332 Extend Documentation of --resilient flag and add warning message when detected

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67675/
---

(Updated June 25, 2018, 3:17 p.m.)


Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Changes
---

Added a fix for a bug in SQLServerManagerImportTest, introduced in SQOOP-: 
while the constructor of the testclass is invoked for every test again and a 
again, the configuration object are only instatiated once. Thus 5 tests were 
reusing the same configuration references, polluting the builders. 
(in the end the builder contained the following tool options: --table-hints 
NOLOCK --table-hints NOLOCK,NOWAIT --non-resilient --resilient)

This fix avoids that by creating a new copy of the builder, each time the 
constructor is invoked


Bugs: SQOOP-3332
https://issues.apache.org/jira/browse/SQOOP-3332


Repository: sqoop-trunk


Description
---

This is the documentation part of SQOOP-.


Diffs (updated)
-

  src/docs/user/connectors.txt f1c7aebe 
  src/java/org/apache/sqoop/manager/SQLServerManager.java c98ad2db 
  src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
cf58f631 
  src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
fc1c4895 


Diff: https://reviews.apache.org/r/67675/diff/2/

Changes: https://reviews.apache.org/r/67675/diff/1-2/


Testing
---

Unit tests, 3rdparty tests, ant docs.

I've also investigated how export and import works: 

Import has it's retry mechanism in 
org.apache.sqoop.mapreduce.db.SQLServerDBRecordReader#nextKeyValue
In case of error, it re-calculates the db query, thus the implicit requirements

Export has it's retry loop in 
org.apache.sqoop.mapreduce.SQLServerAsyncDBExecThread#write
It doesn't recalculate the query, thus is a lot safer.


Thanks,

Fero Szabo

Re: Review Request 67628: Implement an alternative solution for Parquet reading and writing

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67628/#review205279
---



Hi Szabi,

LGTM! :)

What might be a good addition to this patch: a few lines about the new option 
in the corresponding doc. I guess it can be a separate Jira as we usually do it 
like that.


src/test/org/apache/sqoop/TestParquetExport.java
Line 69 (original), 66 (patched)


If the intention here is to run the same test for every implementation, 
then it might make sense to reference the enum class again:

ParquetJobConfiguratorImplementation.values()

.. and automagically have a test for future imlementations.



src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
Lines 104 (patched)


Is null sometimes OK here?

I think it will cause an NPE later down the call stack in 
org.apache.sqoop.avro.AvroUtil#getFileToTest

Though NPEs are straightforward to fix, so up to you whether you want to 
throw something here or not.



src/java/org/apache/sqoop/tool/BaseSqoopTool.java
Line 1912 (original), 1920 (patched)


Where do optionValue and propertyValue come from?

My guess is that option comes from a --flag, and property from a -D... 
property. I guess that the user can somehow declare them in the site.xml as 
well. (?)

Might be an issue:
I see that one overrides the other. Why the duplication? Did you document 
the precedence order?



src/java/org/apache/sqoop/tool/BaseSqoopTool.java
Lines 1935 (patched)


To me this looks like a small detail that can be forgotten to be updated if 
/ when a new implementation is added. 

To avoid it, you might consider using this for supperted values:
Arrays.toString(ParquetJobConfiguratorImplementation.values()


- Fero Szabo


On June 22, 2018, 4:36 p.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67628/
> ---
> 
> (Updated June 22, 2018, 4:36 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3328
> https://issues.apache.org/jira/browse/SQOOP-3328
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> The new implementation uses classes from parquet.hadoop packages.
> TestParquetIncrementalImportMerge has been introduced to cover some gaps we 
> had in the Parquet merge support.
> The test infrastructure is also modified a bit which was needed because of 
> TestParquetIncrementalImportMerge.
> 
> Note that this JIRA does not cover the Hive Parquet import support I will 
> create another JIRA for that.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/SqoopOptions.java 
> d9984af369f901c782b1a74294291819e7d13cdd 
>   src/java/org/apache/sqoop/avro/AvroUtil.java 
> 57c2062568778c5bb53cd4118ce4f030e4ff33f2 
>   src/java/org/apache/sqoop/manager/ConnManager.java 
> c80dd5d9cbaa9b114c12b693e9a686d2cbbe51a3 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 
> 3b5421028d3006e790ed4b711a06dbdb4035b8a0 
>   src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 
> 17c9ed39b1e613a6df36b54cd5395b80e5f8fb0b 
>   src/java/org/apache/sqoop/mapreduce/parquet/ParquetConstants.java 
> ae53a96bddc523a52384715dd97705dc3d9db607 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetExportJobConfigurator.java 
> 8d7b87f6d6832ce8d81d995af4c4bd5eeae38e1b 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetImportJobConfigurator.java 
> fa1bc7d1395fbbbceb3cb72802675aebfdb27898 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorFactory.java
>  ed5103f1d84540ef2fa5de60599e94aa69156abe 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorFactoryProvider.java
>  2286a52030778925349ebb32c165ac062679ff71 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorImplementation.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetMergeJobConfigurator.java 
> 67fdf6602bcbc6c091e1e9bf4176e56658ce5222 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportMapper.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportMapper.java
>  PRE-CREATION 
>   
> 

Re: Review Request 67629: SQOOP-3334 Improve ArgumentArrayBuilder, so arguments are replaceable

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67629/
---

(Updated June 25, 2018, 12:45 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3334
https://issues.apache.org/jira/browse/SQOOP-3334


Repository: sqoop-trunk


Description
---

Changed the implementation so that it uses maps instead of lists.


Diffs (updated)
-

  src/test/org/apache/sqoop/testutil/ArgumentArrayBuilder.java 00ce4fe8 
  src/test/org/apache/sqoop/testutil/TestArgumentArrayBuilder.java PRE-CREATION 


Diff: https://reviews.apache.org/r/67629/diff/3/

Changes: https://reviews.apache.org/r/67629/diff/2-3/


Testing
---

Added 2 new unit tests.
Ran 3rdparty and unit tests.


Thanks,

Fero Szabo

Re: Review Request 67629: SQOOP-3334 Improve ArgumentArrayBuilder, so arguments are replaceable

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67629/
---

(Updated June 22, 2018, 2:08 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3334
https://issues.apache.org/jira/browse/SQOOP-3334


Repository: sqoop-trunk


Description
---

Changed the implementation so that it uses maps instead of lists.


Diffs (updated)
-

  src/test/org/apache/sqoop/testutil/ArgumentArrayBuilder.java 00ce4fe8 
  src/test/org/apache/sqoop/testutil/TestArgumentArrayBuilder.java PRE-CREATION 


Diff: https://reviews.apache.org/r/67629/diff/2/

Changes: https://reviews.apache.org/r/67629/diff/1-2/


Testing
---

Added 2 new unit tests.
Ran 3rdparty and unit tests.


Thanks,

Fero Szabo

Re: Review Request 67689: Use hive executable in (non-JDBC) Hive imports

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67689/#review205234
---



Hi Dani,

I had a look at your patch and it basically looks good to me, it applied 
cleanly on my system and all tests passed.

My only concern is that we lose a bit of test coverage. Wouldn't it make sense 
to reimplement the testcase you deleted in a different way? As far as I can 
see, it was the only test for external hive tables...

It might take some effort to do this though, I didn't have time to understand 
how it works exactly. One might be able to reuse the code in the TestHiveImport 
class.


src/java/org/apache/sqoop/hive/HiveImport.java
Line 330 (original)


Was this config effectively only used in testing and no longer needed?


- Fero Szabo


On June 21, 2018, 1:39 p.m., daniel voros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67689/
> ---
> 
> (Updated June 21, 2018, 1:39 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3323
> https://issues.apache.org/jira/browse/SQOOP-3323
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> When doing Hive imports the old way (not via JDBC that was introduced in 
> SQOOP-3309) we're trying to use the CliDriver class from Hive and fall back 
> to the hive executable (a.k.a. Hive Cli) if that class is not found.
> 
> Since CliDriver and the hive executable that's relying on it are deprecated 
> (see also HIVE-10511), we should switch to using beeline to talk to Hive. 
> With recent additions (e.g. HIVE-18963) this should be easier than before.
> 
> As a first step we could switch to using hive executable. With HIVE-19728 it 
> will be possible (in Hive 3.1) to configure hive to actually run beeline when 
> using the hive executable. This way we could leave it to the user to decide 
> whether to use the deprecated cli or use beeline instead.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/hive/HiveImport.java 5da00a74 
>   src/test/org/apache/sqoop/TestIncrementalImport.java 1ab98021 
>   src/test/org/apache/sqoop/TestSqoopJobDataPublisher.java b3579ac1 
>   src/test/org/apache/sqoop/hive/TestHiveImport.java 436f0e51 
>   
> src/test/org/apache/sqoop/manager/postgresql/PostgresqlExternalTableImportTest.java
>  dd4cfb48 
> 
> 
> Diff: https://reviews.apache.org/r/67689/diff/1/
> 
> 
> Testing
> ---
> 
> run thirdparty and normal UTs, also tested on a cluster
> 
> I'm removing PostgresqlExternalTableImportTest since it was relying on the 
> CliDriver path to do an actual Hive import.
> 
> 
> Thanks,
> 
> daniel voros
> 
>

Review Request 67675: SQOOP-3332 Extend Documentation of --resilient option

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67675/
---

Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3332
https://issues.apache.org/jira/browse/SQOOP-3332


Repository: sqoop-trunk


Description
---

This is the documentation part of SQOOP-.


Diffs
-

  src/docs/user/connectors.txt f1c7aebe 
  src/java/org/apache/sqoop/manager/SQLServerManager.java c98ad2db 
  src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
cf58f631 


Diff: https://reviews.apache.org/r/67675/diff/1/


Testing
---

Unit tests, 3rdparty tests, ant docs.

I've also investigated how export and import works: 

Import has it's retry mechanism in 
org.apache.sqoop.mapreduce.db.SQLServerDBRecordReader#nextKeyValue
In case of error, it re-calculates the db query, thus the implicit requirements

Export has it's retry loop in 
org.apache.sqoop.mapreduce.SQLServerAsyncDBExecThread#write
It doesn't recalculate the query, thus is a lot safer.


Thanks,

Fero Szabo

Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/docs/user/connectors.txt
> > Line 151 (original), 151 (patched)
> > 
> >
> > Shouldn't we just say "This will retry"?

Certainly sounds better.


> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/manager/SQLServerManager.java
> > Line 54 (original), 48 (patched)
> > 
> >
> > There is an extra space here, please remove it.

Done


> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/manager/SQLServerManager.java
> > Line 156 (original)
> > 
> >
> > This input format does not seem to be set by 
> > formatConfigurator.configureContextForImport(context, splitColumn) even if 
> > the splitColumn is null.

Good catch!

This was done because I've deduped parts of the importTable and the importQuery 
functions and only the former executed the statement you refer to.


> On June 19, 2018, 7:16 a.m., Szabolcs Vasas wrote:
> > src/java/org/apache/sqoop/manager/SQLServerManager.java
> > Line 206 (original)
> > 
> >
> > Just to double check: in the new implementation you call 
> > configureConnectionRecoveryForExport instead of 
> > configureConnectionRecoveryForUpdate but that should be fine since in the 
> > original version configureConnectionRecoveryForExport and 
> > configureConnectionRecoveryForUpdate were duplicates, so you dropped 
> > configureConnectionRecoveryForUpdate and kept 
> > configureConnectionRecoveryForExport, right?

yes, those two were duplicates


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204978
---


On June 19, 2018, 10:28 a.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> ---
> 
> (Updated June 19, 2018, 10:28 a.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-
> https://issues.apache.org/jira/browse/SQOOP-
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This change is about changing the default behavior of the MS SQL connector 
> from resilient to non-resilient. I was aiming for the fewest possible 
> modifications while also removed double negation where previously present.
> 
> I've refactored the context configuration into a separate class.
> 
> I've also changed the documentation of the non-resilient flag and added a 
> note about the implicit requirement of the feature (that the split-by column 
> has to be unique and ordered in ascending order). 
> 
> I plan to expand the documentation more in SQOOP-3332, as the (now named) 
> resilient flag works not just for export, but import as well (queries and 
> tables).
> 
> I've also added new tests that cover what classes get loaded in connection 
> with the resilient option. Also, I've refactored SQL Server import tests and 
> added a few more cases for better coverage. (The query import uses a 
> different method and wasn't covered by these tests at all.)
> 
> 
> Diffs
> -
> 
>   src/docs/user/connectors.txt 7c540718 
>   src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742 
>   src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 
>   src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
> c83c2c93 
>   
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67524/diff/5/
> 
> 
> Testing
> ---
> 
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
---

(Updated June 19, 2018, 10:28 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-
https://issues.apache.org/jira/browse/SQOOP-


Repository: sqoop-trunk


Description
---

This change is about changing the default behavior of the MS SQL connector from 
resilient to non-resilient. I was aiming for the fewest possible modifications 
while also removed double negation where previously present.

I've refactored the context configuration into a separate class.

I've also changed the documentation of the non-resilient flag and added a note 
about the implicit requirement of the feature (that the split-by column has to 
be unique and ordered in ascending order). 

I plan to expand the documentation more in SQOOP-3332, as the (now named) 
resilient flag works not just for export, but import as well (queries and 
tables).

I've also added new tests that cover what classes get loaded in connection with 
the resilient option. Also, I've refactored SQL Server import tests and added a 
few more cases for better coverage. (The query import uses a different method 
and wasn't covered by these tests at all.)


Diffs (updated)
-

  src/docs/user/connectors.txt 7c540718 
  src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742 
  src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 
  src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
c83c2c93 
  
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/67524/diff/5/

Changes: https://reviews.apache.org/r/67524/diff/4-5/


Testing
---

Added new unit tests for SqlServerConfigurator.
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.


Thanks,

Fero Szabo

Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > Hi Fero,
> > 
> > Thank you for taking care of this! I think it's always a good idea to avoid 
> > these nagating options. I've posted a few minor issues/questions.
> > 
> > Regards,
> > Daniel

Thanks for the review! I've modified the patch based on your findings.


> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/java/org/apache/sqoop/manager/ExportJobContext.java
> > Lines 38 (patched)
> > 
> >
> > This new constructor is always called with outputFormatClass=null now. 
> > Are you planning on using this later?

removed the unused constructor and modified the patch. I just created it 
thinking I'd use it eventually...


> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
> > Lines 34 (patched)
> > 
> >
> > *"to be resilient"?

thx for catching this


> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
> > Lines 115 (patched)
> > 
> >
> > Could you please add some javadoc about the return value?

Good idea, I just did.


> On June 18, 2018, 12:35 p.m., daniel voros wrote:
> > src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
> > Lines 119 (patched)
> > 
> >
> > Could this be a @Before method since it's called from every TC?

Done.


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204918
---


On June 18, 2018, 2:48 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> ---
> 
> (Updated June 18, 2018, 2:48 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-
> https://issues.apache.org/jira/browse/SQOOP-
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This change is about changing the default behavior of the MS SQL connector 
> from resilient to non-resilient. I was aiming for the fewest possible 
> modifications while also removed double negation where previously present.
> 
> I've refactored the context configuration into a separate class.
> 
> I've also changed the documentation of the non-resilient flag and added a 
> note about the implicit requirement of the feature (that the split-by column 
> has to be unique and ordered in ascending order). 
> 
> I plan to expand the documentation more in SQOOP-3332, as the (now named) 
> resilient flag works not just for export, but import as well (queries and 
> tables).
> 
> I've also added new tests that cover what classes get loaded in connection 
> with the resilient option. Also, I've refactored SQL Server import tests and 
> added a few more cases for better coverage. (The query import uses a 
> different method and wasn't covered by these tests at all.)
> 
> 
> Diffs
> -
> 
>   src/docs/user/connectors.txt 7c540718 
>   src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742 
>   src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 
>   src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
> c83c2c93 
>   
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67524/diff/4/
> 
> 
> Testing
> ---
> 
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
---

(Updated June 18, 2018, 2:48 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-
https://issues.apache.org/jira/browse/SQOOP-


Repository: sqoop-trunk


Description
---

This change is about changing the default behavior of the MS SQL connector from 
resilient to non-resilient. I was aiming for the fewest possible modifications 
while also removed double negation where previously present.

I've refactored the context configuration into a separate class.

I've also changed the documentation of the non-resilient flag and added a note 
about the implicit requirement of the feature (that the split-by column has to 
be unique and ordered in ascending order). 

I plan to expand the documentation more in SQOOP-3332, as the (now named) 
resilient flag works not just for export, but import as well (queries and 
tables).

I've also added new tests that cover what classes get loaded in connection with 
the resilient option. Also, I've refactored SQL Server import tests and added a 
few more cases for better coverage. (The query import uses a different method 
and wasn't covered by these tests at all.)


Diffs (updated)
-

  src/docs/user/connectors.txt 7c540718 
  src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742 
  src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 
  src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
c83c2c93 
  
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/67524/diff/4/

Changes: https://reviews.apache.org/r/67524/diff/3-4/


Testing
---

Added new unit tests for SqlServerConfigurator.
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.


Thanks,

Fero Szabo

Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/#review204926
---




src/docs/user/connectors.txt
Lines 154 (patched)


sounded fine for me as is, but can fix :)



src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
Lines 34 (patched)


great catch!



src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java
Lines 115 (patched)


Sure, make sense



src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
Lines 119 (patched)


make sense


- Fero Szabo


On June 18, 2018, 10:25 a.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67524/
> ---
> 
> (Updated June 18, 2018, 10:25 a.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-
> https://issues.apache.org/jira/browse/SQOOP-
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This change is about changing the default behavior of the MS SQL connector 
> from resilient to non-resilient. I was aiming for the fewest possible 
> modifications while also removed double negation where previously present.
> 
> I've refactored the context configuration into a separate class.
> 
> I've also changed the documentation of the non-resilient flag and added a 
> note about the implicit requirement of the feature (that the split-by column 
> has to be unique and ordered in ascending order). 
> 
> I plan to expand the documentation more in SQOOP-3332, as the (now named) 
> resilient flag works not just for export, but import as well (queries and 
> tables).
> 
> I've also added new tests that cover what classes get loaded in connection 
> with the resilient option. Also, I've refactored SQL Server import tests and 
> added a few more cases for better coverage. (The query import uses a 
> different method and wasn't covered by these tests at all.)
> 
> 
> Diffs
> -
> 
>   src/docs/user/connectors.txt 7c540718 
>   src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742 
>   src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 
>   src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
> c83c2c93 
>   
> src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67524/diff/3/
> 
> 
> Testing
> ---
> 
> Added new unit tests for SqlServerConfigurator.
> unit and 3rd party tests.
> ant docs ran succesfully.
> manual testing.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Re: Review Request 67628: Implement an alternative solution for Parquet reading and writing

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67628/#review204919
---




src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopMergeParquetReducer.java
Lines 30 (patched)






src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportTest.java
Line 145 (original), 147 (patched)


Might be a good idea to remove the Sysout while this is modified. 

Also, you could consider using the new builder pattern, though I see that 
might be better suited for a separate Jira.



src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportTest.java
Line 147 (original), 149 (patched)


So, Hadoop flags are no longer included. Was that your intention here? If 
so, the 'includeHadoopFlags' boolean parameter is confusing for me.



src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportTest.java
Line 147 (original), 149 (patched)






src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java
Lines 669 (patched)


Might not be an issue, but shouldn't this handle dates as well?

I can't think of any other text-like types.

BigDecimals are probably OK.


- Fero Szabo


On June 18, 2018, 9:49 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67628/
> ---
> 
> (Updated June 18, 2018, 9:49 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3328
> https://issues.apache.org/jira/browse/SQOOP-3328
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> The new implementation uses classes from parquet.hadoop packages.
> TestParquetIncrementalImportMerge has been introduced to cover some gaps we 
> had in the Parquet merge support.
> The test infrastructure is also modified a bit which was needed because of 
> TestParquetIncrementalImportMerge.
> 
> Note that this JIRA does not cover the Hive Parquet import support I will 
> create another JIRA for that.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/SqoopOptions.java d9984af36 
>   src/java/org/apache/sqoop/avro/AvroUtil.java 57c206256 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java 3b5421028 
>   src/java/org/apache/sqoop/mapreduce/ImportJobBase.java 17c9ed39b 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/ParquetJobConfiguratorFactoryProvider.java
>  2286a5203 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopMergeParquetReducer.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportJobConfigurator.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetExportMapper.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportJobConfigurator.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetImportMapper.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetJobConfiguratorFactory.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/parquet/hadoop/HadoopParquetMergeJobConfigurator.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/TestBigDecimalExport.java ccea17345 
>   src/test/org/apache/sqoop/TestMerge.java 11806fea6 
>   src/test/org/apache/sqoop/TestParquetExport.java 43dabb57b 
>   src/test/org/apache/sqoop/TestParquetImport.java 27d407aa3 
>   src/test/org/apache/sqoop/TestParquetIncrementalImportMerge.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveImport.java 436f0e512 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java f6d591b73 
>   src/test/org/apache/sqoop/manager/sqlserver/SQLServerHiveImportTest.java 
> e6b086550 
>   src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java a5f85a06b 
>   src/test/org/apache/sqoop/testutil/ImportJobTestCase.java dbefe2097 
>   src/test/org/apache/sqoop/util/ParquetReader.java 56e03a060 
> 
> 
> Diff: https://reviews.apache.org/r/67628/diff/1/
> 
> 
> Testing
> ---
> 
> Ran unit and third party tests successfully.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Review Request 67629: SQOOP-3334 Improve ArgumentArrayBuilder, so arguments are replaceable

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67629/
---

Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-3334
https://issues.apache.org/jira/browse/SQOOP-3334


Repository: sqoop-trunk


Description
---

Changed the implementation so that it uses maps instead of lists.


Diffs
-

  src/test/org/apache/sqoop/testutil/ArgumentArrayBuilder.java 00ce4fe8 
  src/test/org/apache/sqoop/testutil/TestArgumentArrayBuilder.java PRE-CREATION 


Diff: https://reviews.apache.org/r/67629/diff/1/


Testing
---

Added 2 new unit tests.
Ran 3rdparty and unit tests.


Thanks,

Fero Szabo

Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
---

(Updated June 18, 2018, 10:25 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

Added missing Apache header and renamed new class.


Bugs: SQOOP-
https://issues.apache.org/jira/browse/SQOOP-


Repository: sqoop-trunk


Description
---

This change is about changing the default behavior of the MS SQL connector from 
resilient to non-resilient. I was aiming for the fewest possible modifications 
while also removed double negation where previously present.

I've refactored the context configuration into a separate class.

I've also changed the documentation of the non-resilient flag and added a note 
about the implicit requirement of the feature (that the split-by column has to 
be unique and ordered in ascending order). 

I plan to expand the documentation more in SQOOP-3332, as the (now named) 
resilient flag works not just for export, but import as well (queries and 
tables).

I've also added new tests that cover what classes get loaded in connection with 
the resilient option. Also, I've refactored SQL Server import tests and added a 
few more cases for better coverage. (The query import uses a different method 
and wasn't covered by these tests at all.)


Diffs (updated)
-

  src/docs/user/connectors.txt 7c540718 
  src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742 
  src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 
  src/java/org/apache/sqoop/manager/SqlServerManagerContextConfigurator.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
c83c2c93 
  
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerContextConfigurator.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/67524/diff/3/

Changes: https://reviews.apache.org/r/67524/diff/2-3/


Testing
---

Added new unit tests for SqlServerConfigurator.
unit and 3rd party tests.
ant docs ran succesfully.
manual testing.


Thanks,

Fero Szabo

Re: Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
---

(Updated June 18, 2018, 9:26 a.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-
https://issues.apache.org/jira/browse/SQOOP-


Repository: sqoop-trunk


Description (updated)
---

This change is about changing the default behavior of the MS SQL connectore 
from resilient to non-resilient. I was aiming for the fewest possible 
modifications while also removed double negation.

I've also changed the documentation of the non-resilient flag and added a note 
about the implicit requirement of the feature (that the split-by column has to 
be unique and ordered in ascending order). 

I plan to expand the documentation more in SQOOP-3332, as the (now named) 
resilient flag works not just for export, but import as well.

I've also added new tests that cover what classes get loaded in connection with 
the resilient option. Also, I've refactored SQL Server import tests and added a 
few more cases for better coverage. (The query import uses a different method 
and wasn't covered by these tests at all.)


Diffs (updated)
-

  src/docs/user/connectors.txt 7c540718 
  src/java/org/apache/sqoop/manager/ExportJobContext.java 773cf742 
  src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 
  src/java/org/apache/sqoop/manager/SqlServerManagerFormatConfigurator.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/SQLServerManagerImportTest.java 
c83c2c93 
  
src/test/org/apache/sqoop/manager/sqlserver/TestSqlServerManagerConfigurator.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/67524/diff/2/

Changes: https://reviews.apache.org/r/67524/diff/1-2/


Testing
---

unit and 3rd party tests.
ant docs ran succesfully.
manual testing.


Thanks,

Fero Szabo

Review Request 67524: SQOOP-3333 Change default behavior of the MS SQL connector to non-resilient.

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67524/
---

Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-
https://issues.apache.org/jira/browse/SQOOP-


Repository: sqoop-trunk


Description
---

This change is about changing the default behavior of the MS SQL connectore 
from resilient to non-resilient. I was aiming for the fewest possible 
modifications while also removed double negation.

I've also changed the documentation of the non-resilient flag and added a note 
about the implicit requirement of the feature (that the split-by column has to 
be unique and ordered in ascending order). 

I plan to expand the documentation more in SQOOP-3332, as the (now named) 
resilient flag works not just for export, but import as well.


Diffs
-

  src/docs/user/connectors.txt 7c540718 
  src/java/org/apache/sqoop/manager/SQLServerManager.java b136087f 


Diff: https://reviews.apache.org/r/67524/diff/1/


Testing
---

unit and 3rd party tests.
ant docs ran succesfully.
manual testing.


Thanks,

Fero Szabo

Re: Review Request 67407: SQOOP-3327: Mainframe FTP needs to Include "Migrated" datasets when parsing the FTP list

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67407/#review204185
---




src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileEntryParser.java
Line 54 (original), 66 (patched)


Since this is a constant, you could make it final and uppercase (this 
probably applies to all the other constants in this file)

private static final String MIGRATED_STRING = "Migrated";


- Fero Szabo


On May 31, 2018, 11:25 p.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67407/
> ---
> 
> (Updated May 31, 2018, 11:25 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Mainframe FTP needs to Include "Migrated" datasets when parsing the FTP list. 
> Initially, these were excluded out of the regular expression.
> 
> 
> Diffs
> -
> 
>   
> src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileEntryParser.java
>  f0b87868 
>   
> src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileEntryParser.java
>  eb0f8c00 
> 
> 
> Diff: https://reviews.apache.org/r/67407/diff/1/
> 
> 
> Testing
> ---
> 
> Unit testing.
> 
> 
> Thanks,
> 
> Chris Teoh
> 
>

Re: Review Request 62492: SQOOP-3224: Mainframe FTP transfer should have an option to use binary mode for transfer

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62492/#review203967
---



Hi Chris,

I had a quick glance at your patch. Mainly, it seems to be fine for me, with 
some minor issues. 

I'm trying to get new Sqoop features documented. Since this is a new feature, 
can you please add documentation to the project? Just a couple of sentences and 
one or two examples should suffice!
The file to modify is located under src/docs. (It's probably 
src/docs/user/import-mainframe.txt.)

It's unclear to me how I could better test it. I wonder how difficult would it 
be for me to emulate a mainframe on a virtual machine and somehow integrate 
that with sqoop. Certainly sounds like a lot of effort! If more of these 
patches are coming, than we might benefit from an integration test environment 
like this.


src/java/org/apache/sqoop/tool/MainframeImportTool.java
Lines 90 (patched)


What is the unit of the buffersize? 
Your example suggests that it's kB. 
Can you please specify it in the description?



src/java/org/apache/sqoop/tool/MainframeImportTool.java
Lines 218 (patched)


Looks like incorrect parenthesis to me.

Shouldn't it be?
if (SqoopOptions.FileLayout.BinaryFile.equals(options.getFileLayout()) &&
  (
  options.getMainframeInputDatasetName() == null 
  || options.getMainframeInputDatasetName().equals("")
  )
) {
...

Anyway, I recommend using org.apache.commons.lang3.StringUtils.isEmpty() to 
check if a String is null or empty



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 213 (patched)


Why the extra parenthesis around (transfermode)? :)



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 215 (patched)


Just asking out of curiosity, do you know the root cause for this?



src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java
Lines 221 (patched)


Would it make sense to rather throw an exception here and force the user to 
specify the encoding? 

Though I'm not too familiar with FTP... Does it always default to ASCII? In 
that case, I admit that it would make sense to use this as the default.


- Fero Szabo


On May 29, 2018, 8:05 a.m., Chris Teoh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62492/
> ---
> 
> (Updated May 29, 2018, 8:05 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3224
> https://issues.apache.org/jira/browse/SQOOP-3224
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> Added --as-binaryfile and --buffersize to support FTP transfer mode switching.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/SqoopOptions.java d9984af3 
>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java a5962ba4 
>   src/java/org/apache/sqoop/mapreduce/KeyRecordWriters.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/RawKeyTextOutputFormat.java fec34f21 
>   
> src/java/org/apache/sqoop/mapreduce/mainframe/AbstractMainframeDatasetImportMapper.java
>  PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java 
> ea54b07f 
>   
> src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryImportMapper.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryRecord.java
>  PRE-CREATION 
>   
> src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java
>  1f78384b 
>   
> src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetImportMapper.java
>  0b7b5b85 
>   src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java 
> 7e975c7b 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 783651a4 
>   src/java/org/apache/sqoop/tool/ImportTool.java ee79d8b7 
>   src/java/org/apache/sqoop/tool/MainframeImportTool.java 8883301d 
>   src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java 95bc0ecb 
>   
> src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java
>  3547294f 
>   src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 0b0c6c34 
>   src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194 
> 
> 
> Diff: https://reviews.apache.org/r/62492/diff/11/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> Functional testing on mainframe.
> 
> 
> Thanks,
> 
> Chris Teoh
> 
>

Re: Review Request 67086: SQOOP-3324 Document SQOOP-816: Sqoop add support for external Hive tables

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67086/
---

(Updated May 11, 2018, 1:58 p.m.)


Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3324
https://issues.apache.org/jira/browse/SQOOP-3324


Repository: sqoop-trunk


Description
---

This is a missing documentation from Sqoop.


Diffs (updated)
-

  src/docs/man/hive-args.txt 438c1dc4 
  src/docs/user/hive-args.txt 75095641 
  src/docs/user/hive.txt f8f7c27e 


Diff: https://reviews.apache.org/r/67086/diff/2/

Changes: https://reviews.apache.org/r/67086/diff/1-2/


Testing
---

ant docs completed successfully.


Thanks,

Fero Szabo

Re: Review Request 67086: SQOOP-3324 Document SQOOP-816: Sqoop add support for external Hive tables

[Fero Szabo via Review Board]

> On May 11, 2018, 11:32 a.m., daniel voros wrote:
> > src/docs/user/hive.txt
> > Lines 115 (patched)
> > 
> >
> > nit: I'm sure we have this wrong elsewhere too, but I think we should 
> > say "switch" or "option" instead of "flag" if it takes an argument.
> 
> Boglarka Egyed wrote:
> +1 for using "option" here.

Then I have no other options! :)


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67086/#review202923
---


On May 11, 2018, 11:10 a.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67086/
> ---
> 
> (Updated May 11, 2018, 11:10 a.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3324
> https://issues.apache.org/jira/browse/SQOOP-3324
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This is a missing documentation from Sqoop.
> 
> 
> Diffs
> -
> 
>   src/docs/man/hive-args.txt 438c1dc4 
>   src/docs/user/hive-args.txt 75095641 
>   src/docs/user/hive.txt f8f7c27e 
> 
> 
> Diff: https://reviews.apache.org/r/67086/diff/1/
> 
> 
> Testing
> ---
> 
> ant docs completed successfully.
> 
> 
> Thanks,
> 
> Fero Szabo
> 
>

Review Request 67086: SQOOP-3324 Document SQOOP-816: Sqoop add support for external Hive tables

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67086/
---

Review request for Sqoop, Boglarka Egyed, daniel voros, and Szabolcs Vasas.


Bugs: SQOOP-3324
https://issues.apache.org/jira/browse/SQOOP-3324


Repository: sqoop-trunk


Description
---

This is a missing documentation from Sqoop.


Diffs
-

  src/docs/man/hive-args.txt 438c1dc4 
  src/docs/user/hive-args.txt 75095641 
  src/docs/user/hive.txt f8f7c27e 


Diff: https://reviews.apache.org/r/67086/diff/1/


Testing
---

ant docs completed successfully.


Thanks,

Fero Szabo

Re: Review Request 67005: Version differences between ivy configurations

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67005/#review202841
---


Ship it!




Seems ok to me. I've ran unit and 3rd party tests as well.


ivy.xml
Line 90 (original), 90 (patched)


I'm not an expert on ant, so just asking for an educational purpose: what 
does this exactly mean?


- Fero Szabo


On May 8, 2018, 1:43 p.m., daniel voros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67005/
> ---
> 
> (Updated May 8, 2018, 1:43 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3322
> https://issues.apache.org/jira/browse/SQOOP-3322
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> We have multiple ivy configurations defined in ivy.xml.
> 
> - The `redist` configuration is used to select the artifacts that need to be 
> distributed with Sqoop in its tar.gz.
> - The `common` configuration is used to set the classpath during compilation 
> (also refered to as 'hadoop classpath')
> - The `test` configuration is used to set the classpath during junit 
> execution. It extends the `common` config.
> 
> Some artifacts end up having different versions between these three 
> configurations, which means we're using different versions during 
> compilation/testing/runtime.
> 
> 
> Diffs
> -
> 
>   ivy.xml 6af94d9d 
>   ivy/libraries.properties c44b50bc 
> 
> 
> Diff: https://reviews.apache.org/r/67005/diff/1/
> 
> 
> Testing
> ---
> 
> - compared the results of ivy-resolve-hadoop, ivy-resolve-test, 
> ivy-resolve-redist tasks to make sure versions are the same
> - checked unit tests just to be on the safe side, test versions weren't 
> changed though (all passed apart from known issues in SQOOP-3321)
> 
> 
> Thanks,
> 
> daniel voros
> 
>

Re: Review Request 66067: SQOOP-3052: Introduce gradle-based build for Sqoop to make it more developer friendly / open

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66067/#review201828
---




build.gradle
Lines 349 (patched)


When running the docs task, I get 55 warnings and the BUILD FAILED message. 
Can you please fix it? (Should we just ignore the warnings?)

Please get back to me, if you need the logs, I'll run it again.


- Fero Szabo


On April 23, 2018, 10:40 a.m., Anna Szonyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66067/
> ---
> 
> (Updated April 23, 2018, 10:40 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: Sqoop-3052
> https://issues.apache.org/jira/browse/Sqoop-3052
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-3052: Introduce gradle based build for Sqoop to make it more developer 
> friendly / open
> 
> 
> Diffs
> -
> 
>   .gitignore 68cbe28731e613607c208824443d1edf256d9c8a 
>   COMPILING.txt 3b82250488256871352056e9061ad08fabbd7fc5 
>   build.gradle PRE-CREATION 
>   config/checkstyle/checkstyle-java-header.txt PRE-CREATION 
>   config/checkstyle/checkstyle-noframes.xsl PRE-CREATION 
>   config/checkstyle/checkstyle.xml PRE-CREATION 
>   gradle.properties PRE-CREATION 
>   gradle/customUnixStartScript.txt PRE-CREATION 
>   gradle/customWindowsStartScript.txt PRE-CREATION 
>   gradle/sqoop-package.gradle PRE-CREATION 
>   gradle/sqoop-version-gen.gradle PRE-CREATION 
>   gradle/wrapper/gradle-wrapper.jar PRE-CREATION 
>   gradle/wrapper/gradle-wrapper.properties PRE-CREATION 
>   gradlew PRE-CREATION 
>   gradlew.bat PRE-CREATION 
>   settings.gradle PRE-CREATION 
>   src/scripts/rat-violations.sh 1cfbc1502b24dd1b8b7e7ce21f0b5d1880c06556 
>   testdata/hcatalog/conf/hive-site.xml 
> edac7aa9087a84b7a0c660907794adae684ae313 
> 
> 
> Diff: https://reviews.apache.org/r/66067/diff/9/
> 
> 
> Testing
> ---
> 
> ran all new tasks, except for internal maven publishing
> 
> Notes:
> - To try it out you can call ./gradlew tasks --all to see all the tasks and 
> compare them to current tasks/artifacts.
> - Replaced cobertura with jacoco, as it's easier/cleaner to configure, easier 
> to combine all test results into a single report.
> - Generated pom.xml now has correct dependencies/versions
> - Script generation is currently hardcoded and not based on sqoop help, as 
> previously - though added the possiblity of hooking it in later
> 
> 
> Thanks,
> 
> Anna Szonyi
> 
>

Re: Review Request 66067: SQOOP-3052: Introduce gradle-based build for Sqoop to make it more developer friendly / open

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66067/#review201727
---



Hi Anna,

I couldn't apply the patch cleanly, I ran into the following error message:

fero-MBP:sqoop ferencszabo$ git apply ~/Downloads/SQOOP-3052-5.patch 
error: missing binary patch data for 'gradle/wrapper/gradle-wrapper.jar'
error: binary patch does not apply to 'gradle/wrapper/gradle-wrapper.jar'
error: gradle/wrapper/gradle-wrapper.jar: patch does not apply

Managed to apply it with the following command:
patch -p1 -i  ~/Downloads/SQOOP-3052-5.patch

Also, gradle was missing from my system, so I had to install it and then 
gradle/wrapper/gradle-wrapper.jar was probably downloaded after I executed 
'gradle wrapper' or './gradlew wrapper'

I'm not sure how to fix these though. Did I go through the "correct" way to 
install gradle? Maybe I should have invoked './gradlew wrapper' right after 
applying the patch. Would that work without gradle installed on the system?

Anyway thanks for the patch, it would be really cool to use gradle in Sqoop!

- Fero Szabo


On April 18, 2018, 3:10 p.m., Anna Szonyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66067/
> ---
> 
> (Updated April 18, 2018, 3:10 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: Sqoop-3052
> https://issues.apache.org/jira/browse/Sqoop-3052
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-3052: Introduce gradle based build for Sqoop to make it more developer 
> friendly / open
> 
> 
> Diffs
> -
> 
>   .gitignore 68cbe28731e613607c208824443d1edf256d9c8a 
>   COMPILING.txt 3b82250488256871352056e9061ad08fabbd7fc5 
>   build.gradle PRE-CREATION 
>   config/checkstyle/checkstyle-java-header.txt PRE-CREATION 
>   config/checkstyle/checkstyle-noframes.xsl PRE-CREATION 
>   config/checkstyle/checkstyle.xml PRE-CREATION 
>   gradle.properties PRE-CREATION 
>   gradle/customUnixStartScript.txt PRE-CREATION 
>   gradle/customWindowsStartScript.txt PRE-CREATION 
>   gradle/sqoop-package.gradle PRE-CREATION 
>   gradle/sqoop-version-gen.gradle PRE-CREATION 
>   gradle/wrapper/gradle-wrapper.jar PRE-CREATION 
>   gradle/wrapper/gradle-wrapper.properties PRE-CREATION 
>   gradlew PRE-CREATION 
>   gradlew.bat PRE-CREATION 
>   settings.gradle PRE-CREATION 
>   src/scripts/rat-violations.sh 1cfbc1502b24dd1b8b7e7ce21f0b5d1880c06556 
>   testdata/hcatalog/conf/hive-site.xml 
> edac7aa9087a84b7a0c660907794adae684ae313 
> 
> 
> Diff: https://reviews.apache.org/r/66067/diff/8/
> 
> 
> Testing
> ---
> 
> ran all new tasks, except for internal maven publishing
> 
> Notes:
> - To try it out you can call ./gradlew tasks --all to see all the tasks and 
> compare them to current tasks/artifacts.
> - Replaced cobertura with jacoco, as it's easier/cleaner to configure, easier 
> to combine all test results into a single report.
> - Generated pom.xml now has correct dependencies/versions
> - Script generation is currently hardcoded and not based on sqoop help, as 
> previously - though added the possiblity of hooking it in later
> 
> 
> Thanks,
> 
> Anna Szonyi
> 
>

Re: Review Request 66361: Implement HiveServer2 client

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66361/#review201413
---


Ship it!




Ship It!

- Fero Szabo


On April 17, 2018, 10:14 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66361/
> ---
> 
> (Updated April 17, 2018, 10:14 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3309
> https://issues.apache.org/jira/browse/SQOOP-3309
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This JIRA covers the implementation of the client for HiveServer2 and its 
> integration into the classes which use HiveImport.
> 
> - HiveClient interface is introduced with 2 implementation:
>   - HiveImport: this is the original implementation which uses HiveCLI
>   - HiveServer2Client: the new clients which connects to HS2 using JDBC 
> connection
>   - The common code is extracted to HiveCommon class
> - HiveClient should be instantiated using HiveClientFactory which creates and 
> configures the right HiveClient based on the configuration in SqoopOptions
> - HiveMiniCluster is introduced with a couple of helper classes to enable 
> end-to-end HS2 tests
> - A couple of new options are added to SqoopOptions to be able to configure 
> the connection to HS2
> - Validation is implemented for these new options
> 
> 
> Diffs
> -
> 
>   build.xml 7f68b573c65a61150ca78d158084586c87775d84 
>   ivy.xml 6be4fa20fbbf1f303c69d86942b1874e18a14afc 
>   src/docs/user/hive-args.txt 441f54e8e0cee63595937f4e1811abc2d89f9237 
>   src/docs/user/hive.txt 3dc8bb463d602d525fe5f2d07d52cb97efcbab7e 
>   src/java/org/apache/sqoop/SqoopOptions.java 
> 651cebd69ee7e75d06c75945e3607c4fab7eb11c 
>   src/java/org/apache/sqoop/hive/HiveClient.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveClientCommon.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveClientFactory.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveImport.java 
> c2729119d31f7e585f204f2d31b2051eea71b72b 
>   src/java/org/apache/sqoop/hive/HiveServer2Client.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveServer2ConnectionFactory.java 
> PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializer.java 
> PRE-CREATION 
>   src/java/org/apache/sqoop/hive/TableDefWriter.java 
> b7a25b7809e0d50166966a77161dc8ff603fb2d2 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 
> b02e4fe7fda25c7f8171c7db17d15a7987459687 
>   src/java/org/apache/sqoop/tool/CreateHiveTableTool.java 
> d259566180369a55d490144e6f865e728f4f2e61 
>   src/java/org/apache/sqoop/tool/ImportAllTablesTool.java 
> 18f7a0af48d972d5186e9414475e080f1eb765f3 
>   src/java/org/apache/sqoop/tool/ImportTool.java 
> e9920058858653bec7407bf7992eb6445401e813 
>   
> src/test/org/apache/sqoop/hive/HiveServer2ConnectionFactoryInitializerTest.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 
> 8bdc3beb3677312ec0ee2e612616358bca4ca838 
>   src/test/org/apache/sqoop/hive/minicluster/AuthenticationConfiguration.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/hive/minicluster/HiveMiniCluster.java 
> PRE-CREATION 
>   
> src/test/org/apache/sqoop/hive/minicluster/KerberosAuthenticationConfiguration.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/hive/minicluster/NoAuthenticationConfiguration.java 
> PRE-CREATION 
>   
> src/test/org/apache/sqoop/hive/minicluster/PasswordAuthenticationConfiguration.java
>  PRE-CREATION 
>   src/test/org/apache/sqoop/testutil/HiveServer2TestUtil.java PRE-CREATION 
>   src/test/org/apache/sqoop/tool/TestHiveServer2OptionValidations.java 
> PRE-CREATION 
>   src/test/org/apache/sqoop/tool/TestImportTool.java 
> 1c0cf4d863692f75bb8831e834fae47fc18b5df5 
> 
> 
> Diff: https://reviews.apache.org/r/66361/diff/6/
> 
> 
> Testing
> ---
> 
> Ran unit and third party tests suite.
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 66446: SQOOP-2567 SQOOP import for Oracle fails with invalid precision/scale for decimal

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66446/
---

(Updated April 17, 2018, 1:15 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-2567
https://issues.apache.org/jira/browse/SQOOP-2567


Repository: sqoop-trunk


Description
---

This fix allows the user to specify default precision and scale for avro 
schemas. The default values are then used to override "invalid" values, (when 
the database returns 0s as precision) and in case of oracle, the -127 scale 
value. 

**Key points**
- The implementation takes place in the ConnManager#toAvroLogicalType function 
and the overriding funcitons in OraOopConnManager and OracleManager
- Testing is covered very thoroughly by the TestAvroImportForNumericTypes class 
and multiple configurations are used to cover MySQL, Oracle, Postgres and MS 
SQL.

**Implementation specific concerns**
- The edge cases aren't well documented. These tests aim to cover the 
NUMBER/NUMERIC and DECIMAL types with or without specified scale and precision 
thoroughly. Are there any missed testcases?
- The new parameters act as overrides only for PSQL and Oracle databases, 
because we the other databases translate the missing precision to valid values. 
Even though this is true, I've added testcases for MS SQL and MySQL.

- In case of Oracle 
The databae returns if user doesn't specify the default scale and the db return 
-127, we adjust the precision by that much.
Should we throw an exception instead?

- The default precision has to be specified. If it's not there and the database 
returns 0 we throw an exception. 
- Instead, if the default precision and scale aren't there, we could just use 
the maximum possible value i.e. 38 + 127 = 165 as precision and 127 as scale, 
that would fit everything in a very inefficient manner, mostly containing 0s. 
(This also opens up the question whether there is an efficient way to store 
numbers with many 0s in avro.)

**Testing specific concerns**
- The ImportJobTestConfiguration#dropTableIfExists method is not really a test 
configuration related method, however at the time of development, it made sense 
to have it there. This might be better off in another place, such as 
BaseSqoopTest (though I'm unsure how that implementation would look like.)
- The SqlUtil class was created solely to provide a place for the 
executeStatement method. This might also be better off in another class, such 
as BaseSqoopTest.


Diffs (updated)
-

  src/docs/user/import.txt e91a5a84 
  src/java/org/apache/sqoop/avro/AvroUtil.java caed90ea 
  src/java/org/apache/sqoop/config/ConfigurationConstants.java 2197025b 
  src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a6998 
  src/java/org/apache/sqoop/manager/ConnManager.java d88b59bd 
  src/java/org/apache/sqoop/manager/OracleManager.java 929b5061 
  src/java/org/apache/sqoop/manager/SqlManager.java fe997c5f 
  src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java 09207bb4 
  src/java/org/apache/sqoop/manager/oracle/OracleUtils.java aa56e708 
  src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/ImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MSSQLServerImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MySQLImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfigurationForNumber.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationForNumeric.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 PRE-CREATION 
  src/test/org/apache/sqoop/manager/mysql/MySQLLobAvroImportTest.java a6121c9a 
  src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java 75ecc357 
  src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java 
f217f0bc 
  src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java 6d752aa4 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlImportTest.java 
846228a1 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlTestUtil.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java 2220b7d5 
  
src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java 
27dc0cd7 
  src/test/org/apache/sqoop/testutil/SqlUtil.java PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/DatabaseAdapter.java PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/MSSQLServerDatabaseAdapter.java 
PRE-CREATION 
  

Re: Review Request 66446: SQOOP-2567 SQOOP import for Oracle fails with invalid precision/scale for decimal

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66446/
---

(Updated April 17, 2018, 1:11 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-2567
https://issues.apache.org/jira/browse/SQOOP-2567


Repository: sqoop-trunk


Description
---

This fix allows the user to specify default precision and scale for avro 
schemas. The default values are then used to override "invalid" values, (when 
the database returns 0s as precision) and in case of oracle, the -127 scale 
value. 

**Key points**
- The implementation takes place in the ConnManager#toAvroLogicalType function 
and the overriding funcitons in OraOopConnManager and OracleManager
- Testing is covered very thoroughly by the TestAvroImportForNumericTypes class 
and multiple configurations are used to cover MySQL, Oracle, Postgres and MS 
SQL.

**Implementation specific concerns**
- The edge cases aren't well documented. These tests aim to cover the 
NUMBER/NUMERIC and DECIMAL types with or without specified scale and precision 
thoroughly. Are there any missed testcases?
- The new parameters act as overrides only for PSQL and Oracle databases, 
because we the other databases translate the missing precision to valid values. 
Even though this is true, I've added testcases for MS SQL and MySQL.

- In case of Oracle 
The databae returns if user doesn't specify the default scale and the db return 
-127, we adjust the precision by that much.
Should we throw an exception instead?

- The default precision has to be specified. If it's not there and the database 
returns 0 we throw an exception. 
- Instead, if the default precision and scale aren't there, we could just use 
the maximum possible value i.e. 38 + 127 = 165 as precision and 127 as scale, 
that would fit everything in a very inefficient manner, mostly containing 0s. 
(This also opens up the question whether there is an efficient way to store 
numbers with many 0s in avro.)

**Testing specific concerns**
- The ImportJobTestConfiguration#dropTableIfExists method is not really a test 
configuration related method, however at the time of development, it made sense 
to have it there. This might be better off in another place, such as 
BaseSqoopTest (though I'm unsure how that implementation would look like.)
- The SqlUtil class was created solely to provide a place for the 
executeStatement method. This might also be better off in another class, such 
as BaseSqoopTest.


Diffs (updated)
-

  IMPORT_TABLE_1.java PRE-CREATION 
  IMPORT_TABLE_2.java PRE-CREATION 
  IMPORT_TABLE_3.java PRE-CREATION 
  IMPORT_TABLE_4.java PRE-CREATION 
  IMPORT_TABLE_5.java PRE-CREATION 
  IMPORT_TABLE_6.java PRE-CREATION 
  IMPORT_TABLE_7.java PRE-CREATION 
  IMPORT_TABLE_8.java PRE-CREATION 
  src/docs/user/import.txt e91a5a84 
  src/java/org/apache/sqoop/avro/AvroUtil.java caed90ea 
  src/java/org/apache/sqoop/config/ConfigurationConstants.java 2197025b 
  src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a6998 
  src/java/org/apache/sqoop/manager/ConnManager.java d88b59bd 
  src/java/org/apache/sqoop/manager/OracleManager.java 929b5061 
  src/java/org/apache/sqoop/manager/SqlManager.java fe997c5f 
  src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java 09207bb4 
  src/java/org/apache/sqoop/manager/oracle/OracleUtils.java aa56e708 
  src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/ImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MSSQLServerImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MySQLImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfigurationForNumber.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationForNumeric.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 PRE-CREATION 
  src/test/org/apache/sqoop/manager/mysql/MySQLLobAvroImportTest.java a6121c9a 
  src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java 75ecc357 
  src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java 
f217f0bc 
  src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java 6d752aa4 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlImportTest.java 
846228a1 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlTestUtil.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java 2220b7d5 
  

Re: Review Request 66446: SQOOP-2567 SQOOP import for Oracle fails with invalid precision/scale for decimal

[Fero Szabo via Review Board]

> On April 16, 2018, 2:28 p.m., Szabolcs Vasas wrote:
> > src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java
> > Lines 131 (patched)
> > 
> >
> > Now that we have MySqlDatabaseAdapter shouldn't we move this method to 
> > that class?

As we discussed, I'd like to keep the scope of this change smaller, so we can 
do this refactor in another one.


- Fero


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66446/#review201204
---


On April 17, 2018, 12:29 p.m., Fero Szabo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66446/
> ---
> 
> (Updated April 17, 2018, 12:29 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-2567
> https://issues.apache.org/jira/browse/SQOOP-2567
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This fix allows the user to specify default precision and scale for avro 
> schemas. The default values are then used to override "invalid" values, (when 
> the database returns 0s as precision) and in case of oracle, the -127 scale 
> value. 
> 
> **Key points**
> - The implementation takes place in the ConnManager#toAvroLogicalType 
> function and the overriding funcitons in OraOopConnManager and OracleManager
> - Testing is covered very thoroughly by the TestAvroImportForNumericTypes 
> class and multiple configurations are used to cover MySQL, Oracle, Postgres 
> and MS SQL.
> 
> **Implementation specific concerns**
> - The edge cases aren't well documented. These tests aim to cover the 
> NUMBER/NUMERIC and DECIMAL types with or without specified scale and 
> precision thoroughly. Are there any missed testcases?
> - The new parameters act as overrides only for PSQL and Oracle databases, 
> because we the other databases translate the missing precision to valid 
> values. Even though this is true, I've added testcases for MS SQL and MySQL.
> 
> - In case of Oracle 
> The databae returns if user doesn't specify the default scale and the db 
> return -127, we adjust the precision by that much.
> Should we throw an exception instead?
> 
> - The default precision has to be specified. If it's not there and the 
> database returns 0 we throw an exception. 
> - Instead, if the default precision and scale aren't there, we could just use 
> the maximum possible value i.e. 38 + 127 = 165 as precision and 127 as scale, 
> that would fit everything in a very inefficient manner, mostly containing 0s. 
> (This also opens up the question whether there is an efficient way to store 
> numbers with many 0s in avro.)
> 
> **Testing specific concerns**
> - The ImportJobTestConfiguration#dropTableIfExists method is not really a 
> test configuration related method, however at the time of development, it 
> made sense to have it there. This might be better off in another place, such 
> as BaseSqoopTest (though I'm unsure how that implementation would look like.)
> - The SqlUtil class was created solely to provide a place for the 
> executeStatement method. This might also be better off in another class, such 
> as BaseSqoopTest.
> 
> 
> Diffs
> -
> 
>   src/docs/user/import.txt e91a5a84 
>   src/java/org/apache/sqoop/avro/AvroUtil.java caed90ea 
>   src/java/org/apache/sqoop/config/ConfigurationConstants.java 2197025b 
>   src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a6998 
>   src/java/org/apache/sqoop/manager/ConnManager.java d88b59bd 
>   src/java/org/apache/sqoop/manager/OracleManager.java 929b5061 
>   src/java/org/apache/sqoop/manager/SqlManager.java fe997c5f 
>   src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java 09207bb4 
>   src/java/org/apache/sqoop/manager/oracle/OracleUtils.java aa56e708 
>   src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java PRE-CREATION 
>   
> src/test/org/apache/sqoop/configuration/importjob/ImportJobTestConfiguration.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/configuration/importjob/avro/MSSQLServerImportJobTestConfiguration.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/configuration/importjob/avro/MySQLImportJobTestConfiguration.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfiguration.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfigurationForNumber.java
>  PRE-CREATION 
>   
> src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationForNumeric.java
>  PRE-CREATION 
>   
> 

Re: Review Request 66446: SQOOP-2567 SQOOP import for Oracle fails with invalid precision/scale for decimal

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66446/
---

(Updated April 17, 2018, 12:29 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Bugs: SQOOP-2567
https://issues.apache.org/jira/browse/SQOOP-2567


Repository: sqoop-trunk


Description
---

This fix allows the user to specify default precision and scale for avro 
schemas. The default values are then used to override "invalid" values, (when 
the database returns 0s as precision) and in case of oracle, the -127 scale 
value. 

**Key points**
- The implementation takes place in the ConnManager#toAvroLogicalType function 
and the overriding funcitons in OraOopConnManager and OracleManager
- Testing is covered very thoroughly by the TestAvroImportForNumericTypes class 
and multiple configurations are used to cover MySQL, Oracle, Postgres and MS 
SQL.

**Implementation specific concerns**
- The edge cases aren't well documented. These tests aim to cover the 
NUMBER/NUMERIC and DECIMAL types with or without specified scale and precision 
thoroughly. Are there any missed testcases?
- The new parameters act as overrides only for PSQL and Oracle databases, 
because we the other databases translate the missing precision to valid values. 
Even though this is true, I've added testcases for MS SQL and MySQL.

- In case of Oracle 
The databae returns if user doesn't specify the default scale and the db return 
-127, we adjust the precision by that much.
Should we throw an exception instead?

- The default precision has to be specified. If it's not there and the database 
returns 0 we throw an exception. 
- Instead, if the default precision and scale aren't there, we could just use 
the maximum possible value i.e. 38 + 127 = 165 as precision and 127 as scale, 
that would fit everything in a very inefficient manner, mostly containing 0s. 
(This also opens up the question whether there is an efficient way to store 
numbers with many 0s in avro.)

**Testing specific concerns**
- The ImportJobTestConfiguration#dropTableIfExists method is not really a test 
configuration related method, however at the time of development, it made sense 
to have it there. This might be better off in another place, such as 
BaseSqoopTest (though I'm unsure how that implementation would look like.)
- The SqlUtil class was created solely to provide a place for the 
executeStatement method. This might also be better off in another class, such 
as BaseSqoopTest.


Diffs (updated)
-

  src/docs/user/import.txt e91a5a84 
  src/java/org/apache/sqoop/avro/AvroUtil.java caed90ea 
  src/java/org/apache/sqoop/config/ConfigurationConstants.java 2197025b 
  src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a6998 
  src/java/org/apache/sqoop/manager/ConnManager.java d88b59bd 
  src/java/org/apache/sqoop/manager/OracleManager.java 929b5061 
  src/java/org/apache/sqoop/manager/SqlManager.java fe997c5f 
  src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java 09207bb4 
  src/java/org/apache/sqoop/manager/oracle/OracleUtils.java aa56e708 
  src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/ImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MSSQLServerImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MySQLImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfigurationForNumber.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationForNumeric.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 PRE-CREATION 
  src/test/org/apache/sqoop/manager/mysql/MySQLLobAvroImportTest.java a6121c9a 
  src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java 75ecc357 
  src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java 
f217f0bc 
  src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java 6d752aa4 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlImportTest.java 
846228a1 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlTestUtil.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java 2220b7d5 
  
src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java 
27dc0cd7 
  src/test/org/apache/sqoop/testutil/AvroTestUtils.java 75940bf1 
  src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java a5f85a06 
  src/test/org/apache/sqoop/testutil/SqlUtil.java PRE-CREATION 
  

Re: Review Request 66361: Implement HiveServer2 client

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66361/#review201189
---




src/java/org/apache/sqoop/hive/HiveClientFactory.java
Lines 54 (patched)


I think you mentioned that this is package-private visibility so it can be 
used in testing. 

You could consider using the 
org.assertj.core.util.VisibleForTesting
annotation as well, though I'm not sure if that's actually better or worse.



src/java/org/apache/sqoop/hive/HiveServer2Client.java
Lines 88 (patched)


Why not put this into a finally statement? No need to clean up after error?



src/java/org/apache/sqoop/tool/BaseSqoopTool.java
Lines 136 (patched)


I'm fine with abbreviations, however these might confuse those reading the 
code in the long run. It would make it easier for newbies to onboard and help 
less profficient users to understand what a command means if we weren't using 
them.

So, we might want to follow the convention of not using abbreviations in 
the names of fields, constants, methods, etc.

What is your opinion? This might be a good quesiton for the community as 
well, as we don't really have coding guidelines.



src/java/org/apache/sqoop/tool/BaseSqoopTool.java
Line 1854 (original), 1886-1904 (patched)


Nice validation!

You could consider extracting two helper methods here, I believe: 
- validateAllOptionsAreSet(SqoopOption, List optionkeys) throws 
InvalidOptionsException
- validateOnlyOneisSet(SqoopOption, List optionkeys) throws 
InvalidOptionsException

or something similar...

It would help validators reuse your nice error messages in the future!


- Fero Szabo


On April 16, 2018, 9:12 a.m., Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66361/
> ---
> 
> (Updated April 16, 2018, 9:12 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3309
> https://issues.apache.org/jira/browse/SQOOP-3309
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This JIRA covers the implementation of the client for HiveServer2 and its 
> integration into the classes which use HiveImport.
> 
> - HiveClient interface is introduced with 2 implementation:
>   - HiveImport: this is the original implementation which uses HiveCLI
>   - HiveServer2Client: the new clients which connects to HS2 using JDBC 
> connection
>   - The common code is extracted to HiveCommon class
> - HiveClient should be instantiated using HiveClientFactory which creates and 
> configures the right HiveClient based on the configuration in SqoopOptions
> - HiveMiniCluster is introduced with a couple of helper classes to enable 
> end-to-end HS2 tests
> - A couple of new options are added to SqoopOptions to be able to configure 
> the connection to HS2
> - Validation is implemented for these new options
> 
> 
> Diffs
> -
> 
>   build.xml 7f68b573c65a61150ca78d158084586c87775d84 
>   ivy.xml 6be4fa20fbbf1f303c69d86942b1874e18a14afc 
>   src/docs/user/hive-args.txt 441f54e8e0cee63595937f4e1811abc2d89f9237 
>   src/docs/user/hive.txt 3dc8bb463d602d525fe5f2d07d52cb97efcbab7e 
>   src/java/org/apache/sqoop/SqoopOptions.java 
> 651cebd69ee7e75d06c75945e3607c4fab7eb11c 
>   src/java/org/apache/sqoop/hive/HiveClient.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveClientCommon.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveClientFactory.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveImport.java 
> c2729119d31f7e585f204f2d31b2051eea71b72b 
>   src/java/org/apache/sqoop/hive/HiveServer2Client.java PRE-CREATION 
>   src/java/org/apache/sqoop/hive/HiveServer2ConnectionFactory.java 
> PRE-CREATION 
>   src/java/org/apache/sqoop/hive/TableDefWriter.java 
> b7a25b7809e0d50166966a77161dc8ff603fb2d2 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java 
> b02e4fe7fda25c7f8171c7db17d15a7987459687 
>   src/java/org/apache/sqoop/tool/CreateHiveTableTool.java 
> d259566180369a55d490144e6f865e728f4f2e61 
>   src/java/org/apache/sqoop/tool/ImportAllTablesTool.java 
> 18f7a0af48d972d5186e9414475e080f1eb765f3 
>   src/java/org/apache/sqoop/tool/ImportTool.java 
> e9920058858653bec7407bf7992eb6445401e813 
>   src/test/org/apache/sqoop/hive/TestHiveClientFactory.java PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveMiniCluster.java PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveServer2Client.java PRE-CREATION 
>   src/test/org/apache/sqoop/hive/TestHiveServer2TextImport.java PRE-CREATION 
>   

Re: Review Request 66446: SQOOP-2567 SQOOP import for Oracle fails with invalid precision/scale for decimal

[Fero Szabo via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66446/
---

(Updated April 13, 2018, 3:45 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Changes
---

I also added documentation for this new feature


Bugs: SQOOP-2567
https://issues.apache.org/jira/browse/SQOOP-2567


Repository: sqoop-trunk


Description
---

This fix allows the user to specify default precision and scale for avro 
schemas. The default values are then used to override "invalid" values, (when 
the database returns 0s as precision) and in case of oracle, the -127 scale 
value. 

**Key points**
- The implementation takes place in the ConnManager#toAvroLogicalType function 
and the overriding funcitons in OraOopConnManager and OracleManager
- Testing is covered very thoroughly by the TestAvroImportForNumericTypes class 
and multiple configurations are used to cover MySQL, Oracle, Postgres and MS 
SQL.

**Implementation specific concerns**
- The edge cases aren't well documented. These tests aim to cover the 
NUMBER/NUMERIC and DECIMAL types with or without specified scale and precision 
thoroughly. Are there any missed testcases?
- The new parameters act as overrides only for PSQL and Oracle databases, 
because we the other databases translate the missing precision to valid values. 
Even though this is true, I've added testcases for MS SQL and MySQL.

- In case of Oracle 
The databae returns if user doesn't specify the default scale and the db return 
-127, we adjust the precision by that much.
Should we throw an exception instead?

- The default precision has to be specified. If it's not there and the database 
returns 0 we throw an exception. 
- Instead, if the default precision and scale aren't there, we could just use 
the maximum possible value i.e. 38 + 127 = 165 as precision and 127 as scale, 
that would fit everything in a very inefficient manner, mostly containing 0s. 
(This also opens up the question whether there is an efficient way to store 
numbers with many 0s in avro.)

**Testing specific concerns**
- The ImportJobTestConfiguration#dropTableIfExists method is not really a test 
configuration related method, however at the time of development, it made sense 
to have it there. This might be better off in another place, such as 
BaseSqoopTest (though I'm unsure how that implementation would look like.)
- The SqlUtil class was created solely to provide a place for the 
executeStatement method. This might also be better off in another class, such 
as BaseSqoopTest.


Diffs (updated)
-

  src/docs/user/import.txt e91a5a84 
  src/java/org/apache/sqoop/config/ConfigurationConstants.java 2197025b 
  src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a6998 
  src/java/org/apache/sqoop/manager/ConnManager.java d88b59bd 
  src/java/org/apache/sqoop/manager/OracleManager.java 929b5061 
  src/java/org/apache/sqoop/manager/SqlManager.java fe997c5f 
  src/java/org/apache/sqoop/manager/oracle/OraOopConnManager.java 09207bb4 
  src/java/org/apache/sqoop/manager/oracle/OracleUtils.java aa56e708 
  src/test/org/apache/sqoop/AvroImportForNumericTypesTest.java PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/ImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MSSQLServerImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/MySQLImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfiguration.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/OracleImportJobTestConfigurationForNumber.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationForNumeric.java
 PRE-CREATION 
  
src/test/org/apache/sqoop/configuration/importjob/avro/PostgresqlImportJobTestConfigurationPaddingShouldSucceed.java
 PRE-CREATION 
  src/test/org/apache/sqoop/manager/mysql/MySQLLobAvroImportTest.java a6121c9a 
  src/test/org/apache/sqoop/manager/mysql/MySQLTestUtils.java 75ecc357 
  src/test/org/apache/sqoop/manager/oracle/OracleAvroPaddingImportTest.java 
f217f0bc 
  src/test/org/apache/sqoop/manager/oracle/util/OracleUtils.java 6d752aa4 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlImportTest.java 
846228a1 
  src/test/org/apache/sqoop/manager/postgresql/PostgresqlTestUtil.java 
PRE-CREATION 
  src/test/org/apache/sqoop/manager/sqlserver/MSSQLTestUtils.java 2220b7d5 
  
src/test/org/apache/sqoop/manager/sqlserver/SQLServerAvroPaddingImportTest.java 
27dc0cd7 
  src/test/org/apache/sqoop/testutil/BaseSqoopTestCase.java a5f85a06 
  src/test/org/apache/sqoop/testutil/SqlUtil.java PRE-CREATION 
  src/test/org/apache/sqoop/testutil/adapter/DatabaseAdapter.java PRE-CREATION