You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Boglarka Egyed <eg...@gmail.com> on 2016/10/24 14:03:45 UTC
Review Request 53134: HBase import should fail fast if using anything
other than as-textfile
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: SQOOP-3034
https://issues.apache.org/jira/browse/SQOOP-3034
Repository: sqoop-trunk
Description
-------
HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
Diffs
-----
src/java/com/cloudera/sqoop/SqoopOptions.java f4ababeada9af866e65b5f11124451d39a81e6ff
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
Diff: https://reviews.apache.org/r/53134/diff/
Testing
-------
Test cases have been added for option validation check.
Thanks,
Boglarka Egyed
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Boglarka Egyed <eg...@gmail.com>.
> On Oct. 24, 2016, 4:14 p.m., Attila Szabo wrote:
> > src/java/com/cloudera/sqoop/SqoopOptions.java, lines 46-59
> > <https://reviews.apache.org/r/53134/diff/2/?file=1544216#file1544216line46>
> >
> > Hi,
> >
> > This "arg" property does not look very helpful/expressive here.
> >
> > Could you please rename it like "formatName" or something similar?
Final solution does not contain it.
> On Oct. 24, 2016, 4:14 p.m., Attila Szabo wrote:
> > src/java/com/cloudera/sqoop/SqoopOptions.java, lines 46-49
> > <https://reviews.apache.org/r/53134/diff/2/?file=1544216#file1544216line46>
> >
> > I'm a bit concerned here. Do we really want to notify the user with the name of the argument instead with the name of the format?
> >
> > Could you please highlight me the pros?
Thanks for the feedback, it was a valid point to think about, I have reverted this change.
> On Oct. 24, 2016, 4:14 p.m., Attila Szabo wrote:
> > src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java, lines 91-144
> > <https://reviews.apache.org/r/53134/diff/2/?file=1544218#file1544218line91>
> >
> > Could you please squash these for test cases into "one" and instead use the DDT capabilities of JUnit to solve the parametrization on that level?
Thanks for the idea, I have made a parametrized test case for the failing cases.
- Boglarka
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/#review153699
-----------------------------------------------------------
On Nov. 6, 2016, 6:01 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53134/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2016, 6:01 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-3034
> https://issues.apache.org/jira/browse/SQOOP-3034
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
> src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
>
> Diff: https://reviews.apache.org/r/53134/diff/
>
>
> Testing
> -------
>
> Test cases have been added for option validation check.
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Attila Szabo <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/#review153699
-----------------------------------------------------------
H
src/java/com/cloudera/sqoop/SqoopOptions.java (lines 46 - 59)
<https://reviews.apache.org/r/53134/#comment223117>
Hi,
This "arg" property does not look very helpful/expressive here.
Could you please rename it like "formatName" or something similar?
src/java/com/cloudera/sqoop/SqoopOptions.java (lines 46 - 49)
<https://reviews.apache.org/r/53134/#comment223121>
I'm a bit concerned here. Do we really want to notify the user with the name of the argument instead with the name of the format?
Could you please highlight me the pros?
src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java (lines 91 - 144)
<https://reviews.apache.org/r/53134/#comment223122>
Could you please squash these for test cases into "one" and instead use the DDT capabilities of JUnit to solve the parametrization on that level?
Hi Bogi,
I think introducing this functionality totally make sense, the users must be aware about this limitation, and it most not happen silently Sqoop does something very different.
Although I've found a few places where you could improve this changeset. Could you please check and fix them?
Thanks,
Maugli
- Attila Szabo
On Oct. 24, 2016, 3:49 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53134/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2016, 3:49 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-3034
> https://issues.apache.org/jira/browse/SQOOP-3034
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
>
>
> Diffs
> -----
>
> src/java/com/cloudera/sqoop/SqoopOptions.java f4ababeada9af866e65b5f11124451d39a81e6ff
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
> src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
>
> Diff: https://reviews.apache.org/r/53134/diff/
>
>
> Testing
> -------
>
> Test cases have been added for option validation check.
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Anna Szonyi <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/#review155306
-----------------------------------------------------------
Ship it!
Ship It!
- Anna Szonyi
On Nov. 8, 2016, 2:53 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53134/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2016, 2:53 p.m.)
>
>
> Review request for Sqoop and Attila Szabo.
>
>
> Bugs: SQOOP-3034
> https://issues.apache.org/jira/browse/SQOOP-3034
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
> src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
>
> Diff: https://reviews.apache.org/r/53134/diff/
>
>
> Testing
> -------
>
> Test cases have been added for option validation check.
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Attila Szabo <as...@cloudera.com>.
> On Nov. 8, 2016, 5:08 p.m., Attila Szabo wrote:
> > Ship It!
Thank you Bogi for applying the changes!
It really made it better, nice job!
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/#review155304
-----------------------------------------------------------
On Nov. 8, 2016, 2:53 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53134/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2016, 2:53 p.m.)
>
>
> Review request for Sqoop and Attila Szabo.
>
>
> Bugs: SQOOP-3034
> https://issues.apache.org/jira/browse/SQOOP-3034
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
> src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
>
> Diff: https://reviews.apache.org/r/53134/diff/
>
>
> Testing
> -------
>
> Test cases have been added for option validation check.
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Attila Szabo <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/#review155304
-----------------------------------------------------------
Ship it!
Ship It!
- Attila Szabo
On Nov. 8, 2016, 2:53 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53134/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2016, 2:53 p.m.)
>
>
> Review request for Sqoop and Attila Szabo.
>
>
> Bugs: SQOOP-3034
> https://issues.apache.org/jira/browse/SQOOP-3034
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
>
>
> Diffs
> -----
>
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
> src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
>
> Diff: https://reviews.apache.org/r/53134/diff/
>
>
> Testing
> -------
>
> Test cases have been added for option validation check.
>
>
> Thanks,
>
> Boglarka Egyed
>
>
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Boglarka Egyed <eg...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/
-----------------------------------------------------------
(Updated Nov. 8, 2016, 2:53 p.m.)
Review request for Sqoop and Attila Szabo.
Changes
-------
Minor change in the junit import.
Bugs: SQOOP-3034
https://issues.apache.org/jira/browse/SQOOP-3034
Repository: sqoop-trunk
Description
-------
HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
Diffs (updated)
-----
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
Diff: https://reviews.apache.org/r/53134/diff/
Testing
-------
Test cases have been added for option validation check.
Thanks,
Boglarka Egyed
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Boglarka Egyed <eg...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/
-----------------------------------------------------------
(Updated Nov. 6, 2016, 6:01 p.m.)
Review request for Sqoop.
Bugs: SQOOP-3034
https://issues.apache.org/jira/browse/SQOOP-3034
Repository: sqoop-trunk
Description
-------
HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
Diffs (updated)
-----
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
Diff: https://reviews.apache.org/r/53134/diff/
Testing
-------
Test cases have been added for option validation check.
Thanks,
Boglarka Egyed
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Boglarka Egyed <eg...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/
-----------------------------------------------------------
(Updated Oct. 24, 2016, 3:49 p.m.)
Review request for Sqoop.
Bugs: SQOOP-3034
https://issues.apache.org/jira/browse/SQOOP-3034
Repository: sqoop-trunk
Description
-------
HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
Diffs (updated)
-----
src/java/com/cloudera/sqoop/SqoopOptions.java f4ababeada9af866e65b5f11124451d39a81e6ff
src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
Diff: https://reviews.apache.org/r/53134/diff/
Testing
-------
Test cases have been added for option validation check.
Thanks,
Boglarka Egyed
Re: Review Request 53134: HBase import should fail fast if using
anything other than as-textfile
Posted by Szabolcs Vasas <va...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53134/#review153692
-----------------------------------------------------------
src/java/com/cloudera/sqoop/SqoopOptions.java (line 23)
<https://reviews.apache.org/r/53134/#comment223111>
We usually avoid wildcard imports, can you please organize it?
src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java (line 9)
<https://reviews.apache.org/r/53134/#comment223112>
We usually avoid wildcard imports, can you please organize it?
src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java (line 88)
<https://reviews.apache.org/r/53134/#comment223113>
The new test cases look good only the names should be fixed. E.g. testValidationSucceedWithHBaseImportAndAsSequenceFiles should be testValidationFailsWithHBaseImportAndAsSequenceFilebecause the test throws an exception.
- Szabolcs Vasas
On Oct. 24, 2016, 2:03 p.m., Boglarka Egyed wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53134/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2016, 2:03 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-3034
> https://issues.apache.org/jira/browse/SQOOP-3034
>
>
> Repository: sqoop-trunk
>
>
> Description
> -------
>
> HBase import should fail fast if using anything other than as-textfile as those are ignored in the current implementation, however, the user is not informed about it and the HBase import performs silently as a textfile.
>
>
> Diffs
> -----
>
> src/java/com/cloudera/sqoop/SqoopOptions.java f4ababeada9af866e65b5f11124451d39a81e6ff
> src/java/org/apache/sqoop/tool/BaseSqoopTool.java 13a9697b2d94e3846f86e1b605cfff3d01dd9347
> src/test/org/apache/sqoop/tool/ImportToolValidateOptionsTest.java 503a86340f5ae33c8eb0a27b8450b2af0a342db9
>
> Diff: https://reviews.apache.org/r/53134/diff/
>
>
> Testing
> -------
>
> Test cases have been added for option validation check.
>
>
> Thanks,
>
> Boglarka Egyed
>
>