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

2018-03-22 Thread Anna Szonyi via Review Board 

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

(Updated March 22, 2018, 8:53 p.m.)


Review request for Sqoop.


Changes
---

added sourcecompatibilityversion, do not copy .gradle to packaging, gitignore 
.gradle


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 (updated)
-

  .gitignore 68cbe28 
  COMPILING.txt 86be509 
  build.gradle PRE-CREATION 
  buildSrc/customUnixStartScript.txt PRE-CREATION 
  buildSrc/customWindowsStartScript.txt PRE-CREATION 
  buildSrc/sqoop-package.gradle PRE-CREATION 
  buildSrc/sqoop-version-gen.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/wrapper/gradle-wrapper.jar PRE-CREATION 
  gradle/wrapper/gradle-wrapper.properties PRE-CREATION 
  gradlew PRE-CREATION 
  gradlew.bat PRE-CREATION 
  settings.gradle PRE-CREATION 


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

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


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

2018-03-22 Thread Anna Szonyi via Review Board 


> On March 22, 2018, 4:19 p.m., Boglarka Egyed wrote:
> > gradlew
> > Lines 1 (patched)
> > 
> >
> > License information header is missing from this file.

I don't think we can add the license header to this file, as it's the auto 
generated gradle "runner" file.


- Anna


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


On March 22, 2018, 6:32 p.m., Anna Szonyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66067/
> ---
> 
> (Updated March 22, 2018, 6:32 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 68cbe28 
>   COMPILING.txt 86be509 
>   build.gradle PRE-CREATION 
>   buildSrc/customUnixStartScript.txt PRE-CREATION 
>   buildSrc/customWindowsStartScript.txt PRE-CREATION 
>   buildSrc/sqoop-package.gradle PRE-CREATION 
>   buildSrc/sqoop-version-gen.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/wrapper/gradle-wrapper.jar PRE-CREATION 
>   gradle/wrapper/gradle-wrapper.properties PRE-CREATION 
>   gradlew PRE-CREATION 
>   gradlew.bat PRE-CREATION 
>   settings.gradle PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66067/diff/2/
> 
> 
> 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

2018-03-22 Thread Anna Szonyi via Review Board 

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

(Updated March 22, 2018, 6:32 p.m.)


Review request for Sqoop.


Changes
---

changed per Bogi's comment + added the missing gradle/gradle-wrapper.jar


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 (updated)
-

  .gitignore 68cbe28 
  COMPILING.txt 86be509 
  build.gradle PRE-CREATION 
  buildSrc/customUnixStartScript.txt PRE-CREATION 
  buildSrc/customWindowsStartScript.txt PRE-CREATION 
  buildSrc/sqoop-package.gradle PRE-CREATION 
  buildSrc/sqoop-version-gen.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/wrapper/gradle-wrapper.jar PRE-CREATION 
  gradle/wrapper/gradle-wrapper.properties PRE-CREATION 
  gradlew PRE-CREATION 
  gradlew.bat PRE-CREATION 
  settings.gradle PRE-CREATION 


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

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


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

2018-03-22 Thread Szabolcs Vasas 

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



Hi Anna,

Thank you for the patch, it makes the contribution much easier!
I will continue my review tomorrow, I just wanted to post the findings from 
today.

1) The gradle wrapper script does not find the gradle-wrapper.jar, can you 
please add it to the patch as well 
(https://docs.gradle.org/current/dsl/org.gradle.api.tasks.wrapper.Wrapper.html)?
2) Once the gradle-wrapper.jar is present the ./gradlew test runs successfully
3) The project can be imported to IntelliJ without any problems
4) I think a couple of more items should be added to .gitignore
   .gradle/ -> I think this is generated by the gradle wrapper
   out/ -> I am not sure why but when I import the project to IntelliJ and run 
a test from there it generates the out folder which was not present with the 
ant build
5) Can we define the sourceCompatibility in build.gradle to say that we support 
1.7 currently?
6) I have executed the package target with both ant and gradle and I found some 
differences, can you please check that these are expected?
   In case of gradle build bat files are also generated for sqoop tools scripts.
   The .gradle folder is copied to 
build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0, I am not sure this is desired.
   Some library versions are different in case of ant and gradle build.

I am posting the relevant lines from the diff output:
Only in buildgradle/bin: sqoop-codegen.bat
Only in buildgradle/bin: sqoop-create-hive-table.bat
Only in buildgradle/bin: sqoop-eval.bat
Only in buildgradle/bin: sqoop-export.bat
Only in buildgradle/bin: sqoop-help.bat
Only in buildgradle/bin: sqoop-import-all-tables.bat
Only in buildgradle/bin: sqoop-import-mainframe.bat
Only in buildgradle/bin: sqoop-import.bat
Only in buildgradle/bin: sqoop-job.bat
Only in buildgradle/bin: sqoop-list-databases.bat
Only in buildgradle/bin: sqoop-list-tables.bat
Only in buildgradle/bin: sqoop-merge.bat
Only in buildgradle/bin: sqoop-metastore.bat
Only in buildgradle/bin: sqoop-version.bat
Only in build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1: 
file-changes
Binary files 
build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/fileContent/fileContent.lock
 and 
buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/fileContent/fileContent.lock
 differ
Binary files 
build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileHashes.bin
 and 
buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileHashes.bin
 differ
Binary files 
build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileSnapshots.bin
 and 
buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/fileSnapshots.bin
 differ
Binary files 
build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.bin
 and 
buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.bin
 differ
Binary files 
build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.lock
 and 
buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/.gradle/3.5.1/taskHistory/taskHistory.lock
 differ
Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: 
avro-ipc-1.8.1.jar
Only in build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: 
commons-codec-1.4.jar
Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: 
commons-codec-1.9.jar
Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: 
parquet-hive-bundle-1.6.0.jar
Only in build/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: 
slf4j-api-1.6.1.jar
Only in buildgradle/sqoop-1.5.0-SNAPSHOT.bin__hadoop-2.6.0/lib: 
slf4j-api-1.7.7.jar


COMPILING.txt
Lines 206 (patched)


typo: gradlew



COMPILING.txt
Lines 215 (patched)


typo: gradlew



config/checkstyle/checkstyle-noframes.xsl
Lines 200 (patched)


There are 2 new lines at the end of this line, git gives a warning when the 
patch is applied.


- Szabolcs Vasas


On March 19, 2018, 1:55 p.m., Anna Szonyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66067/
> ---
> 
> (Updated March 19, 2018, 1:55 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 /

Review Request 66221: SQOOP-3301 Document SQOOP-3216 - metastore related change

2018-03-22 Thread Fero Szabo via Review Board 

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

Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


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


Repository: sqoop-trunk


Description
---

This is the documentation for the metastore related patch implemented by Zach 
Berkowitz.


Diffs
-

  src/docs/man/sqoop-metastore.txt c10cc08d 
  src/docs/user/metastore-purpose.txt 95c2d774 
  src/docs/user/saved-jobs.txt 6885079f 


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


Testing
---

ant docs ran successfully


Thanks,

Fero Szabo

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

2018-03-22 Thread Fero Szabo via Review Board 


> On March 22, 2018, 4:19 p.m., Boglarka Egyed wrote:
> > Hi Anna,
> > 
> > This is a superb improvement in Sqoop, many thanks for taking this huge 
> > effort! Using Gradle would indeed ease the life of the developers as well 
> > as let more enhancements to come in the future.
> > 
> > However I was not able to apply your patch, I got the following error 
> > message:
> > 
> > error: patch failed: COMPILING.txt:403
> > error: COMPILING.txt: patch does not apply
> > /Users/boglarka.egyed/Downloads/SQOOP-3052.patch:1178: new blank line at 
> > EOF.
> > 
> > Could you please update the patch file?
> > 
> > I also took a first glance and found some minor things, please find them 
> > below.
> > 
> > Many thanks,
> > Bogi

Hi Bogi, Anna,

This is possibly coming from my patch, as I edited compiling.txt. 
https://reviews.apache.org/r/66062/ 

Though reviewboard doesn't show it, so I'm a bit puzzled. Also, my patch 
probably applied without problems.


- Fero


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


On March 19, 2018, 1:55 p.m., Anna Szonyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66067/
> ---
> 
> (Updated March 19, 2018, 1:55 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 68cbe28 
>   COMPILING.txt 86be509 
>   build.gradle PRE-CREATION 
>   buildSrc/customUnixStartScript.txt PRE-CREATION 
>   buildSrc/customWindowsStartScript.txt PRE-CREATION 
>   buildSrc/sqoop-package.gradle PRE-CREATION 
>   buildSrc/sqoop-version-gen.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 
>   gradlew PRE-CREATION 
>   gradlew.bat PRE-CREATION 
>   settings.gradle PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66067/diff/1/
> 
> 
> 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 66062: SQOOP-3293 Documentation for avro padding (SQOOP-2976)

2018-03-22 Thread Fero Szabo via Review Board 

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

(Updated March 22, 2018, 4:25 p.m.)


Review request for Sqoop, Boglarka Egyed and Szabolcs Vasas.


Summary (updated)
-

SQOOP-3293 Documentation for avro padding (SQOOP-2976)


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


Repository: sqoop-trunk


Description
---

Documentation for avro decimal padding i.e. the 
sqoop.avro.logical_types.decimal.enable flag.

I also added a few lines to mention sqoop.avro.logical_types.decimal.enable, 
because the two goes hand in hand.

Also fixed 2 typos.


Diffs
-

  COMPILING.txt 86be5094 
  src/docs/user/import.txt 330d5445 


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


Testing
---


Thanks,

Fero Szabo

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

2018-03-22 Thread Boglarka Egyed 

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



Hi Anna,

This is a superb improvement in Sqoop, many thanks for taking this huge effort! 
Using Gradle would indeed ease the life of the developers as well as let more 
enhancements to come in the future.

However I was not able to apply your patch, I got the following error message:

error: patch failed: COMPILING.txt:403
error: COMPILING.txt: patch does not apply
/Users/boglarka.egyed/Downloads/SQOOP-3052.patch:1178: new blank line at EOF.

Could you please update the patch file?

I also took a first glance and found some minor things, please find them below.

Many thanks,
Bogi


COMPILING.txt
Lines 206 (patched)


Typo: gralew



COMPILING.txt
Lines 215 (patched)


Typo: gralew



gradlew
Lines 1 (patched)


License information header is missing from this file.


- Boglarka Egyed


On March 19, 2018, 1:55 p.m., Anna Szonyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66067/
> ---
> 
> (Updated March 19, 2018, 1:55 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 68cbe28 
>   COMPILING.txt 86be509 
>   build.gradle PRE-CREATION 
>   buildSrc/customUnixStartScript.txt PRE-CREATION 
>   buildSrc/customWindowsStartScript.txt PRE-CREATION 
>   buildSrc/sqoop-package.gradle PRE-CREATION 
>   buildSrc/sqoop-version-gen.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 
>   gradlew PRE-CREATION 
>   gradlew.bat PRE-CREATION 
>   settings.gradle PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66067/diff/1/
> 
> 
> 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
> 
>