Re: Review Request 66282: Mock ConnManager field in TestTableDefWriter

2018-03-28 Thread daniel voros 

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


Ship it!




Ship It!

- daniel voros


On March 27, 2018, noon, Szabolcs Vasas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66282/
> ---
> 
> (Updated March 27, 2018, noon)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3308
> https://issues.apache.org/jira/browse/SQOOP-3308
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> This patch removes the externalColTypes field from TableDefWriter since it 
> was only used for testing purposes.
> TestTableDefWriter is fixed to mock the ConnManager object provided to the 
> TableDefWriter constructor and a minor refactoring is done on the class.
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/hive/TableDefWriter.java e1424c383 
>   src/test/org/apache/sqoop/hive/TestTableDefWriter.java 496b5add9 
> 
> 
> Diff: https://reviews.apache.org/r/66282/diff/4/
> 
> 
> Testing
> ---
> 
> ant clean test
> 
> 
> Thanks,
> 
> Szabolcs Vasas
> 
>

Re: Review Request 66300: Upgrade to Hadoop 3.0.0

2018-03-28 Thread Szabolcs Vasas 

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



Hi Dani,

Thank you for starting this initiative! The changes look good to me, ran the 
unit and third party tests successfully, I have left some minor comments only.
I guess we do not want to commit this patch until the final version of Hive 3 
is released, am I right? Do you know when it is going to happen?


ivy.xml
Lines 95 (patched)


Can we move the version to libraries.properties?



src/java/org/apache/sqoop/hive/HiveImport.java
Lines 368 (patched)


Is it possible that before setting DerbyPolicy another Policy was set? In 
that case I think we should restore the original Policy.



src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java
Lines 69 (patched)


Can we use List interface and diamond operator here?



src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java
Lines 71 (patched)


Missing @Override annotation.



src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java
Lines 75 (patched)


Missing @Override annotation.



src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java
Lines 76 (patched)


Can we use for-each loop here?



src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java
Lines 84 (patched)


Missing @Override annotation.



src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java
Lines 88 (patched)


Missing @Override annotation.



src/java/org/apache/sqoop/util/SqoopJsonUtil.java
Line 42 (original), 42 (patched)


Casing: getJsonStringForMap



src/java/org/apache/sqoop/util/SqoopJsonUtil.java
Lines 44 (patched)


It is a matter of taste, I am not a big fan of changing the parameter, 
maybe we could change it to something like this:

if (map == null) {
return EMPTY_JSON_MAP;
}

where EMPTY_JSON_MAP would be a constant extracted from isEmptyJSON method 
("{}")


- Szabolcs Vasas


On March 27, 2018, 8:50 a.m., daniel voros wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66300/
> ---
> 
> (Updated March 27, 2018, 8:50 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-3305
> https://issues.apache.org/jira/browse/SQOOP-3305
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> To be able to eventually support the latest versions of Hive, HBase and 
> Accumulo, we should start by upgrading our Hadoop dependencies to 3.0.0. See 
> https://hadoop.apache.org/docs/r3.0.0/index.html
> 
> 
> Diffs
> -
> 
>   ivy.xml 6be4fa2 
>   ivy/libraries.properties c44b50b 
>   src/java/org/apache/sqoop/config/ConfigurationHelper.java e07a699 
>   src/java/org/apache/sqoop/hive/HiveImport.java c272911 
>   src/java/org/apache/sqoop/mapreduce/JobBase.java 6d1e049 
>   src/java/org/apache/sqoop/mapreduce/hcat/DerbyPolicy.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/hcat/SqoopHCatUtilities.java 784b5f2 
>   src/java/org/apache/sqoop/util/SqoopJsonUtil.java adf186b 
>   src/test/org/apache/sqoop/TestSqoopOptions.java bb7c20d 
>   testdata/hcatalog/conf/hive-site.xml edac7aa 
> 
> 
> Diff: https://reviews.apache.org/r/66300/diff/1/
> 
> 
> Testing
> ---
> 
> Normal and third-party unit tests.
> 
> 
> Thanks,
> 
> daniel voros
> 
>

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

2018-03-28 Thread daniel voros 

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



Hey Anna,

I've experimented with running the gradle build in a clean dockerized 
environment and found some minor issues:
 1) All dependencies are downloaded from jcenter, despite having central 
repository defined in build.gradle. This might be a result of my current setup, 
could you please confirm?
 2) Deprecation warnings for '<<' task doLast syntax ("Deprecation warning: The 
Task.leftShift(Closure) method has been deprecated...")
 3) SqoopVersion.java generation happens during task definition and not in 
action (missing doLast?).
 4) `relnotes` task fails if version is SNAPSHOT with: "A problem occurred 
starting process 'command 'cd''".
 5) The `release` task prints the path of tar and rat report but they're 
incorrect. (I've specified version on the command line with "-Pversion=1.5.0")
 6) `ant releaseaudit` now lists gradle files as errors

I've corrected 2,3,4,5,6 in this commit: 
https://github.com/dvoros/sqoop/commit/47e361829b1004bdedd6f5c223332e3fb8b85696

What's the reasoning behind using Gradle 3.5.1? Shouldn't we use 4.x? (I've 
successfully executed a simple build with 4.6)

Regards,
Daniel

- daniel voros


On March 23, 2018, 10:28 a.m., Anna Szonyi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66067/
> ---
> 
> (Updated March 23, 2018, 10:28 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 68cbe28 
>   COMPILING.txt 3b82250 
>   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/6/
> 
> 
> 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
> 
>