Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Attila Szabo]

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


Ship it!




Hey Ying Cao,

Both implementation and testing looks good. Thanks for your contrubtion here! 
(Hope more will come in the future!)

Regards,
Attila

- Attila Szabo


On Feb. 24, 2017, 6:56 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated Feb. 24, 2017, 6:56 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

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

(Updated 二月 24, 2017, 6:56 a.m.)


Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.


Changes
---

Fix issue base on Szabolcs comments


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


Repository: sqoop-trunk


Description
---

SQOOP-1904: support for DB2 XML data type when importing to hdfs


Diffs (updated)
-

  src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
  src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/55132/diff/


Testing
---

UT passed by manually


Thanks,

Ying Cao

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Szabolcs Vasas]

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



Hi Ying,

Thank you for the patch! I think the code you added works correctly, however 
there are a few minor things need to be fixed. See my comments below.

Regards,
Szabolcs


src/java/org/apache/sqoop/manager/Db2Manager.java (line 53)


I think this constant should have a more expressive name, as it contains 
the Java type the XML type is mapped to.



src/java/org/apache/sqoop/manager/Db2Manager.java (line 131)


Can we move this field declaration above the constructor? That would be 
inline with Java code style.



src/java/org/apache/sqoop/manager/Db2Manager.java (line 168)


I am not sure the debug message would be correct here. The keySet() method 
returns a set which is not necessarily ordered, so the debug message can be 
misleading.



src/java/org/apache/sqoop/manager/Db2Manager.java (line 175)


You use startsWith instead of equals here. Do we have more XML data types?



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (line 213)


The test looks good to me, however then I try to run it it fails, because 
test.build.data does not seem to be defined on my machine. Can you please fix 
it?


- Szabolcs Vasas


On Feb. 9, 2017, 3:39 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated Feb. 9, 2017, 3:39 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

> On 二月 8, 2017, 10:25 p.m., Abraham Elmahrek wrote:
> >

Hi Abraham Elmahrek,

Thanks for your comments

Angela


- Ying


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


On 二月 9, 2017, 3:39 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated 二月 9, 2017, 3:39 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

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

(Updated 二月 9, 2017, 3:39 a.m.)


Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.


Changes
---

Update patch SQOOP-1904.6.patch wtih  Abraham's comments


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


Repository: sqoop-trunk


Description
---

SQOOP-1904: support for DB2 XML data type when importing to hdfs


Diffs (updated)
-

  src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
  src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/55132/diff/


Testing
---

UT passed by manually


Thanks,

Ying Cao

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

> On 二月 8, 2017, 10:25 p.m., Abraham Elmahrek wrote:
> > src/java/org/apache/sqoop/manager/Db2Manager.java, line 176
> > 
> >
> > Is this a Hive specific type? Or a Java specific type?

Hi Abraham Elmahrek,
Thanks for your comments.
This is Java specific type


> On 二月 8, 2017, 10:25 p.m., Abraham Elmahrek wrote:
> > src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java, 
> > lines 218-220
> > 
> >
> > Is this necessary?

Thanks for your comments.


> On 二月 8, 2017, 10:25 p.m., Abraham Elmahrek wrote:
> > src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java, line 
> > 215
> > 
> >
> > You could just move the contents of this method into the test case 
> > method above

Thanks for comments!

This method will help for resued if other more testing scenario is need.


- Ying


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


On 二月 6, 2017, 3:19 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated 二月 6, 2017, 3:19 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Abraham Elmahrek]

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




src/java/org/apache/sqoop/manager/Db2Manager.java (line 176)


Is this a Hive specific type? Or a Java specific type?



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (line 190)


Why not just use a static string or static array of strings?



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (line 215)


You could just move the contents of this method into the test case method 
above



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (lines 
218 - 220)


Is this necessary?


- Abraham Elmahrek


On Feb. 6, 2017, 3:19 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated Feb. 6, 2017, 3:19 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

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

(Updated 二月 6, 2017, 3:19 a.m.)


Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.


Changes
---

Hi Bogi,

Thanks for your comments again,  name of "JAVA_STRING_DATA_TYPE" is more clean 
than before.


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


Repository: sqoop-trunk


Description
---

SQOOP-1904: support for DB2 XML data type when importing to hdfs


Diffs (updated)
-

  src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
  src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/55132/diff/


Testing
---

UT passed by manually


Thanks,

Ying Cao

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Boglarka Egyed]

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



Hi Ying,

I have one last minor finding, please correct it.

Also, could you please open a documentation Jira ticket linked to the original 
one? I think this is an improvement which would be nice to document.

Many thanks,
Bogi


src/java/org/apache/sqoop/manager/Db2Manager.java (line 53)


Just a little modification: could you please include the word "string" into 
the name too? It would me more clean e.g. as JAVA_STRING_DATA_TYPE or so.


- Boglarka Egyed


On Jan. 23, 2017, 6:17 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated Jan. 23, 2017, 6:17 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

> On 一月 18, 2017, 5:52 p.m., Boglarka Egyed wrote:
> > src/java/org/apache/sqoop/manager/Db2Manager.java, line 53
> > 
> >
> > Thanks for creating a constant for this, however, it would be great to 
> > check if there is already an enum or something for Hive data types and use 
> > it if so.

Hi Bogi,

Sorry to confused, this is an JAVA DATA TYPE, I have update name to 
JAVA_DATA_TYPE.


- Ying


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


On 一月 23, 2017, 6:17 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated 一月 23, 2017, 6:17 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

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

(Updated 一月 23, 2017, 6:17 a.m.)


Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.


Changes
---

Hi Bogi,

Thanks for your comments.

I have rebased my patch with latest trunk and with fix base on your comments, 
please kindly review it again, thanks!


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


Repository: sqoop-trunk


Description
---

SQOOP-1904: support for DB2 XML data type when importing to hdfs


Diffs (updated)
-

  src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
  src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/55132/diff/


Testing
---

UT passed by manually


Thanks,

Ying Cao

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Boglarka Egyed]

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



Hi Angela,

Thank you for the corrections, I have two more findings based on the new diff 
file. Also, a committer also needs to check your change who is able to push it 
into the repo.

Thanks,
Bogi


src/java/org/apache/sqoop/manager/Db2Manager.java (line 53)


Thanks for creating a constant for this, however, it would be great to 
check if there is already an enum or something for Hive data types and use it 
if so.



src/test/com/cloudera/sqoop/ThirdPartyTests.java (lines 47 - 141)


Meanwhile this file has been removed by SQOOP-3099, be aware of this and 
make your change mergeable.


- Boglarka Egyed


On Jan. 13, 2017, 9:46 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated Jan. 13, 2017, 9:46 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/com/cloudera/sqoop/ThirdPartyTests.java 7e10c68 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Ying Cao]

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

(Updated 一月 13, 2017, 9:46 a.m.)


Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.


Changes
---

Hi Bogi,

Thanks for you comments, I have fix them and update diff file, please help 
review it again

Thanks
Angela


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


Repository: sqoop-trunk


Description
---

SQOOP-1904: support for DB2 XML data type when importing to hdfs


Diffs (updated)
-

  src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
  src/test/com/cloudera/sqoop/ThirdPartyTests.java 7e10c68 
  src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/55132/diff/


Testing
---

UT passed by manually


Thanks,

Ying Cao

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Boglarka Egyed]

> On Jan. 8, 2017, 12:57 p.m., Boglarka Egyed wrote:
> >

Hi Ying,

I have some findings regarding your change, please find them below.

Thanks,
Bogi


- Boglarka


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


On Jan. 3, 2017, 9:39 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated Jan. 3, 2017, 9:39 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/com/cloudera/sqoop/ThirdPartyTests.java 7e10c68 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>

Re: Review Request 55132: support for DB2 XML data type when importing to hdfs

[Boglarka Egyed]

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




src/java/org/apache/sqoop/manager/Db2Manager.java (line 144)


I might miss a point here but could you please clarify what does "$ 
shiju" stand for in the message? Also, writing to STDOUT won't appear in the 
log, you should use org.apache.commons.logging.Log and 
org.apache.commons.logging.LogFactory as you did it in the added test case.



src/java/org/apache/sqoop/manager/Db2Manager.java (line 172)


Please make sure you remove unnecessary white spaces.
You can easily spot them here in the Review Board with checking the "View 
Diff" view where they show up with red highlighting.



src/java/org/apache/sqoop/manager/Db2Manager.java (line 175)


I would recommend to use a named constant instead of "String" to make the 
code more clean and undertandable.



src/java/org/apache/sqoop/manager/Db2Manager.java (line 219)


I would recommend to use a named constant instead of "String" to make the 
code more clean and undertandable.



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (line 34)


Please avoid using wildcard imports based on the common Sqoop coding 
guidelines.



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (lines 
154 - 155)


Including the exception itself here will also include the stack trace, 
there is no need for calling the printStackTrace method separately.



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (line 174)


It would be a little more useful to include the exception itself here not 
just the name of it as it would also include it's stack trace.



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (lines 
233 - 234)


Same here with the stack trace.



src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java (lines 
248 - 249)


Same here with the stack trace.


- Boglarka Egyed


On Jan. 3, 2017, 9:39 a.m., Ying Cao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55132/
> ---
> 
> (Updated Jan. 3, 2017, 9:39 a.m.)
> 
> 
> Review request for Sqoop, Abraham Elmahrek and Jarek Cecho.
> 
> 
> Bugs: SQOOP-1904
> https://issues.apache.org/jira/browse/SQOOP-1904
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> ---
> 
> SQOOP-1904: support for DB2 XML data type when importing to hdfs
> 
> 
> Diffs
> -
> 
>   src/java/org/apache/sqoop/manager/Db2Manager.java e39aa4c 
>   src/test/com/cloudera/sqoop/ThirdPartyTests.java 7e10c68 
>   src/test/org/apache/sqoop/manager/db2/DB2XmlTypeImportManualTest.java 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55132/diff/
> 
> 
> Testing
> ---
> 
> UT passed by manually
> 
> 
> Thanks,
> 
> Ying Cao
> 
>