You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Ying Cao <an...@gmail.com> on 2017/04/21 02:44:40 UTC
Review Request 58600: add --schema option for import-all-tables and
list-tables against db2
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/
-----------------------------------------------------------
Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
Bugs: SQOOP-1905
https://issues.apache.org/jira/browse/SQOOP-1905
Repository: sqoop-trunk
Description
-------
SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
Diffs
-----
src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION
Diff: https://reviews.apache.org/r/58600/diff/1/
Testing
-------
Mannual UT is passed
Thanks,
Ying Cao
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Ying Cao <an...@gmail.com>.
> On 五月 17, 2017, 11:50 a.m., Attila Szabo wrote:
> > src/java/org/apache/sqoop/manager/Db2Manager.java
> > Lines 90-94 (patched)
> > <https://reviews.apache.org/r/58600/diff/1/?file=1696010#file1696010line90>
> >
> > Please fix indentation + formatting here!
Hi Attila
Thanks a lot for your detail suggestions
Angela
> On 五月 17, 2017, 11:50 a.m., Attila Szabo wrote:
> > src/java/org/apache/sqoop/manager/Db2Manager.java
> > Lines 181 (patched)
> > <https://reviews.apache.org/r/58600/diff/1/?file=1696010#file1696010line181>
> >
> > Is this commit call neded here?
> > We've just only executed SELECT statement. What kind of changes we'd like to commit?
Hi Attila
Thanks for your suggestion, I have remove this commit and rollback code for "SELECT" statement
Angela
> On 五月 17, 2017, 11:50 a.m., Attila Szabo wrote:
> > src/java/org/apache/sqoop/manager/Db2Manager.java
> > Lines 372-394 (patched)
> > <https://reviews.apache.org/r/58600/diff/1/?file=1696010#file1696010line372>
> >
> > Could you please check if this has been already solved in other connectors?
> >
> > Getting schema option sounds quite general, might have been implemented elsewhere in the codebase.
> >
> > Would make sense to abstract it out, and just call it from different places.
> >
> > Thanks for checking!
Hi Attila
Yes, this has been already solved in Postgresql. Which file is suitable to abstrace it out?
- Ying
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review175233
-----------------------------------------------------------
On 五月 24, 2017, 9:20 a.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated 五月 24, 2017, 9:20 a.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/2/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Attila Szabo <ma...@apache.org>.
> On May 17, 2017, 11:50 a.m., Attila Szabo wrote:
> > src/java/org/apache/sqoop/manager/Db2Manager.java
> > Lines 372-394 (patched)
> > <https://reviews.apache.org/r/58600/diff/1/?file=1696010#file1696010line372>
> >
> > Could you please check if this has been already solved in other connectors?
> >
> > Getting schema option sounds quite general, might have been implemented elsewhere in the codebase.
> >
> > Would make sense to abstract it out, and just call it from different places.
> >
> > Thanks for checking!
>
> Ying Cao wrote:
> Hi Attila
>
> Yes, this has been already solved in Postgresql. Which file is suitable to abstrace it out?
Hey Angela,
I would recommend GenericJdbcManager class.
Cheers,
Attila
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review175233
-----------------------------------------------------------
On May 24, 2017, 9:20 a.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 9:20 a.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/2/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Ying Cao <an...@gmail.com>.
> On 五月 17, 2017, 11:50 a.m., Attila Szabo wrote:
> > src/java/org/apache/sqoop/manager/Db2Manager.java
> > Lines 372-394 (patched)
> > <https://reviews.apache.org/r/58600/diff/1/?file=1696010#file1696010line372>
> >
> > Could you please check if this has been already solved in other connectors?
> >
> > Getting schema option sounds quite general, might have been implemented elsewhere in the codebase.
> >
> > Would make sense to abstract it out, and just call it from different places.
> >
> > Thanks for checking!
>
> Ying Cao wrote:
> Hi Attila
>
> Yes, this has been already solved in Postgresql. Which file is suitable to abstrace it out?
>
> Attila Szabo wrote:
> Hey Angela,
>
> I would recommend GenericJdbcManager class.
>
> Cheers,
> Attila
Hi Attila,
Thanks for your suggestions.
I have abstrace it in GenericJdbcManager class
Angela
- Ying
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review175233
-----------------------------------------------------------
On 六月 4, 2017, 4:38 p.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated 六月 4, 2017, 4:38 p.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/java/org/apache/sqoop/manager/GenericJdbcManager.java 2113a5f5
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManual.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/3/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Attila Szabo <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review175233
-----------------------------------------------------------
Fix it, then Ship it!
Hey Angela,
Many thanks for providing this contribution.
In general your changeset looks good, I've just found some codestyle issues, JDBC connection handling, and logging conventions.
Could you please check them, and apply the changes / answer where I've raised questions?
After we have the amendments I will be able to commit your changes!
Many thanks,
Attila
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 90-94 (patched)
<https://reviews.apache.org/r/58600/#comment248686>
Please fix indentation + formatting here!
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 181 (patched)
<https://reviews.apache.org/r/58600/#comment248687>
Is this commit call neded here?
We've just only executed SELECT statement. What kind of changes we'd like to commit?
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 184 (patched)
<https://reviews.apache.org/r/58600/#comment248688>
I have the same concerns as around commit. Could you please give some more insights around why is this call needed? (FYI: I'm not a DB2 expert, so it's very possible I'm just not aware about the internals)
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 229 (patched)
<https://reviews.apache.org/r/58600/#comment248674>
Please remove the trailing whitesapce!
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 246-249 (patched)
<https://reviews.apache.org/r/58600/#comment248689>
Same concerns with commit+rollback as above
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 372-394 (patched)
<https://reviews.apache.org/r/58600/#comment248685>
Could you please check if this has been already solved in other connectors?
Getting schema option sounds quite general, might have been implemented elsewhere in the codebase.
Would make sense to abstract it out, and just call it from different places.
Thanks for checking!
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 382 (patched)
<https://reviews.apache.org/r/58600/#comment248675>
Please remove trailing whitesapces!
src/java/org/apache/sqoop/manager/Db2Manager.java
Lines 393 (patched)
<https://reviews.apache.org/r/58600/#comment248676>
Please remove trailing whitesapces!
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 153 (patched)
<https://reviews.apache.org/r/58600/#comment248677>
Please remove trailing whitesapces!
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 166 (patched)
<https://reviews.apache.org/r/58600/#comment248683>
Please include debug level stack trace logging here!
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 233 (patched)
<https://reviews.apache.org/r/58600/#comment248682>
Please include debug level stack trace logging!
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 245 (patched)
<https://reviews.apache.org/r/58600/#comment248681>
Please include debug level stacktrace loggging here as well!
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 258 (patched)
<https://reviews.apache.org/r/58600/#comment248680>
How is this change related to the oraoop module?
AFAIK DB2 execution path has to be independent from the oraoop execution path.
What kind of problem did you face which led you to include this config settings?
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 264 (patched)
<https://reviews.apache.org/r/58600/#comment248679>
Please do remove printStackTrace calls.
Logging the exception is the conventional way in the Sqoop codebase.
If you'd like to add the stacktrace to the log as well (which is a best practice and very much advised), please add another line like:
LOG.debug("Sqoop exception details", e);
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 270 (patched)
<https://reviews.apache.org/r/58600/#comment248678>
Please fix indendtation according to Sqoop guideline (here two leading spaces are missing)
- Attila Szabo
On April 21, 2017, 2:44 a.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated April 21, 2017, 2:44 a.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/1/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Attila Szabo <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review181105
-----------------------------------------------------------
Ship it!
Ship It!
- Attila Szabo
On June 5, 2017, 3:01 p.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated June 5, 2017, 3:01 p.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/java/org/apache/sqoop/manager/GenericJdbcManager.java 2113a5f5
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManualTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/4/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Ying Cao <an...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/
-----------------------------------------------------------
(Updated 六月 5, 2017, 3:01 p.m.)
Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
Changes
-------
Fix base on Bogi's comments
Bugs: SQOOP-1905
https://issues.apache.org/jira/browse/SQOOP-1905
Repository: sqoop-trunk
Description
-------
SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
Diffs (updated)
-----
src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
src/java/org/apache/sqoop/manager/GenericJdbcManager.java 2113a5f5
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManualTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/58600/diff/4/
Changes: https://reviews.apache.org/r/58600/diff/3-4/
Testing
-------
Mannual UT is passed
Thanks,
Ying Cao
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Ying Cao <an...@gmail.com>.
> On 六月 5, 2017, 11:09 a.m., Boglarka Egyed wrote:
> > src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManual.java
> > Lines 76 (patched)
> > <https://reviews.apache.org/r/58600/diff/3/?file=1741029#file1741029line76>
> >
> > Please use ManualTest at the end so that the related ant task for manual tests will find it.
Hi Bogi,
Thanks for your remind, I have update fix, please help review it again.
Regards,
Angela
- Ying
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review176905
-----------------------------------------------------------
On 六月 5, 2017, 3:01 p.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated 六月 5, 2017, 3:01 p.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/java/org/apache/sqoop/manager/GenericJdbcManager.java 2113a5f5
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManualTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/4/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Boglarka Egyed <eg...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review176905
-----------------------------------------------------------
Fix it, then Ship it!
Hi Angela,
Build and ant clean test were successful with your patch.
I have left one last finding, please correct it before committing this change.
Thanks for your contribution!
Cheers,
Bogi
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManual.java
Lines 76 (patched)
<https://reviews.apache.org/r/58600/#comment250457>
Please use ManualTest at the end so that the related ant task for manual tests will find it.
- Boglarka Egyed
On June 4, 2017, 4:38 p.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated June 4, 2017, 4:38 p.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/java/org/apache/sqoop/manager/GenericJdbcManager.java 2113a5f5
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManual.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/3/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Ying Cao <an...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/
-----------------------------------------------------------
(Updated 六月 4, 2017, 4:38 p.m.)
Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
Bugs: SQOOP-1905
https://issues.apache.org/jira/browse/SQOOP-1905
Repository: sqoop-trunk
Description
-------
SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
Diffs (updated)
-----
src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
src/java/org/apache/sqoop/manager/GenericJdbcManager.java 2113a5f5
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManual.java PRE-CREATION
Diff: https://reviews.apache.org/r/58600/diff/3/
Changes: https://reviews.apache.org/r/58600/diff/2-3/
Testing
-------
Mannual UT is passed
Thanks,
Ying Cao
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Attila Szabo <ma...@apache.org>.
> On May 24, 2017, 3:15 p.m., Boglarka Egyed wrote:
> > Hi Ying,
> >
> > I tried to run 'ant clean test' with your patch but ran into some compilation errors. Please find the details below.
> >
> > Thanks,
> > Bogi
Hey Bogi,
Thanks for checking, and catching these issues!
Angela! Please also fix the issues what Bogi raised, and around the logging you could also consider to do some generalization (as I can see you're using LoggingUtils and the Logger itself in a kinda mixed way, depending you your personal preferences it would be nice, to use only one of these ways for logging).
Thanks,
Attila
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review175936
-----------------------------------------------------------
On May 24, 2017, 9:20 a.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 9:20 a.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/2/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Ying Cao <an...@gmail.com>.
> On 五月 24, 2017, 3:15 p.m., Boglarka Egyed wrote:
> > Hi Ying,
> >
> > I tried to run 'ant clean test' with your patch but ran into some compilation errors. Please find the details below.
> >
> > Thanks,
> > Bogi
>
> Attila Szabo wrote:
> Hey Bogi,
>
> Thanks for checking, and catching these issues!
>
> Angela! Please also fix the issues what Bogi raised, and around the logging you could also consider to do some generalization (as I can see you're using LoggingUtils and the Logger itself in a kinda mixed way, depending you your personal preferences it would be nice, to use only one of these ways for logging).
>
> Thanks,
> Attila
Hi Attila,
I have fix issues Bogi raised, thanks Bogi detials. The manual test is passed
And I have make some updates use loggingUtils to pop-up SQLException, Logger for others.
> On 五月 24, 2017, 3:15 p.m., Boglarka Egyed wrote:
> > src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
> > Lines 73 (patched)
> > <https://reviews.apache.org/r/58600/diff/2/?file=1731302#file1731302line73>
> >
> > Could you please include "ManualTest" at the end of this test name to follow the usual naming convention which is also used in the build.xml to create the manual test suite?
Hi Bogi,
Thnaks for your comments
Angela
- Ying
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review175936
-----------------------------------------------------------
On 六月 5, 2017, 3:01 p.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated 六月 5, 2017, 3:01 p.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/java/org/apache/sqoop/manager/GenericJdbcManager.java 2113a5f5
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchemaManualTest.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/4/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Boglarka Egyed <eg...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/#review175936
-----------------------------------------------------------
Hi Ying,
I tried to run 'ant clean test' with your patch but ran into some compilation errors. Please find the details below.
Thanks,
Bogi
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 73 (patched)
<https://reviews.apache.org/r/58600/#comment249275>
Could you please include "ManualTest" at the end of this test name to follow the usual naming convention which is also used in the build.xml to create the manual test suite?
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 142 (patched)
<https://reviews.apache.org/r/58600/#comment249276>
This line causes a compilation error because of the ",":
error: incompatible types: String cannot be converted to Throwable [javac] LOG.warn("Exception while closing stmt", ex.getMessage());
Please use the exception itself instead of the getMessage - could you please correct it at all the other occasions as well?
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 162 (patched)
<https://reviews.apache.org/r/58600/#comment249277>
This line causes a compilation error because of the ",":
error: incompatible types: String cannot be converted to Throwable [javac] LOG.error("Encountered SQL Exception: ", sqlE.getMessage());
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java
Lines 169 (patched)
<https://reviews.apache.org/r/58600/#comment249278>
This line causes a compilation error because of the ",":
error: incompatible types: String cannot be converted to Throwable [javac] LOG.warn("Exception while closing connection/stmt", ex.getMessage());
- Boglarka Egyed
On May 24, 2017, 9:20 a.m., Ying Cao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58600/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 9:20 a.m.)
>
>
> Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
>
>
> Bugs: SQOOP-1905
> https://issues.apache.org/jira/browse/SQOOP-1905
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
> src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/58600/diff/2/
>
>
> Testing
> -------
>
> Mannual UT is passed
>
>
> Thanks,
>
> Ying Cao
>
>
Re: Review Request 58600: add --schema option for import-all-tables
and list-tables against db2
Posted by Ying Cao <an...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58600/
-----------------------------------------------------------
(Updated 五月 24, 2017, 9:20 a.m.)
Review request for Sqoop, Abraham Elmahrek, Boglarka Egyed, Jarek Cecho, Attila Szabo, and Szabolcs Vasas.
Changes
-------
Update patch with Attila's comments
Bugs: SQOOP-1905
https://issues.apache.org/jira/browse/SQOOP-1905
Repository: sqoop-trunk
Description
-------
SQOOP-1905 : add --schema option for import-all-tables and list-tables against db2
Diffs (updated)
-----
src/java/org/apache/sqoop/manager/Db2Manager.java 52ab05ef
src/test/org/apache/sqoop/manager/db2/DB2ImportAllTableWithSchema.java PRE-CREATION
Diff: https://reviews.apache.org/r/58600/diff/2/
Changes: https://reviews.apache.org/r/58600/diff/1-2/
Testing
-------
Mannual UT is passed
Thanks,
Ying Cao