You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Erzsebet Szilagyi <li...@cloudera.com> on 2016/05/26 20:56:34 UTC
Review Request 47917: Sqoop2: Support getting connectors by Direction
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47917/
-----------------------------------------------------------
Review request for Sqoop.
Bugs: https://issues.apache.org/jira/browse/SQOOP-1531
https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/SQOOP-1531
Repository: sqoop-sqoop2
Description
-------
Added functions to shell and client to support getting connectors listed by direction
Diffs
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e
client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java adf4eb5
shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java c90fe94
shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a5f5d4d
shell/src/main/resources/shell-resource.properties 2c94c58
shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
Diff: https://reviews.apache.org/r/47917/diff/
Testing
-------
Added unit tests.
Both unit and integration tests run successfully.
Thanks,
Erzsebet Szilagyi
Re: Review Request 47917: Sqoop2: Support getting connectors by
Direction
Posted by Abraham Fine <ab...@abrahamfine.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47917/#review135085
-----------------------------------------------------------
great work! I left a few minor suggestions.
client/src/main/java/org/apache/sqoop/client/SqoopClient.java (line 35)
<https://reviews.apache.org/r/47917/#comment200077>
my ide shows this import as unused
client/src/main/java/org/apache/sqoop/client/SqoopClient.java (line 218)
<https://reviews.apache.org/r/47917/#comment200080>
nit-pick: this can be rewritten as Collection<MConnector> filteredConnectors = new ArrayList<>();
client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java (line 192)
<https://reviews.apache.org/r/47917/#comment200086>
it would be great if this test could include a connector that supports both "to" and "from"
client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java (line 196)
<https://reviews.apache.org/r/47917/#comment200088>
nit-pick: i think you can just make these inner ArrayLists null, may make this slightly less verbose.
shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java (line 21)
<https://reviews.apache.org/r/47917/#comment200093>
my ide shows some of these new imports as unused.
shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java (line 109)
<https://reviews.apache.org/r/47917/#comment200094>
nit-pick: we have a style of having a space after the // in comments
shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java (line 111)
<https://reviews.apache.org/r/47917/#comment200095>
nit-pick: the = null is not needed.
shell/src/main/resources/shell-resource.properties (line 145)
<https://reviews.apache.org/r/47917/#comment200099>
perhaps reword as "that support the given direction". "with direction" does not sound right outside the context of the way the connectors are implemented.
shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java (line 226)
<https://reviews.apache.org/r/47917/#comment200104>
i don't think this is the correct error message
- Abraham Fine
On May 26, 2016, 9:46 p.m., Erzsebet Szilagyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47917/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 9:46 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1531
> https://issues.apache.org/jira/browse/SQOOP-1531
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Added functions to shell and client to support getting connectors listed by direction
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e
> client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java adf4eb5
> shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java c90fe94
> shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a5f5d4d
> shell/src/main/resources/shell-resource.properties 2c94c58
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
>
> Diff: https://reviews.apache.org/r/47917/diff/
>
>
> Testing
> -------
>
> Added unit tests.
> Both unit and integration tests run successfully.
>
>
> Thanks,
>
> Erzsebet Szilagyi
>
>
Re: Review Request 47917: Sqoop2: Support getting connectors by
Direction
Posted by Erzsebet Szilagyi <li...@cloudera.com>.
> On May 27, 2016, 12:51 a.m., Abraham Fine wrote:
> > client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java, line 214
> > <https://reviews.apache.org/r/47917/diff/1-3/?file=1395804#file1395804line214>
> >
> > what do you think about using assertFalse instead of assertTrue(!...)?
- assertFalse requires another import in this file;
- personally i feel like using assertTrue in these lines gives a more consistent picture than having both assertFalse and assertTrue.
- Erzsebet
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47917/#review135103
-----------------------------------------------------------
On May 27, 2016, 12:40 a.m., Erzsebet Szilagyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47917/
> -----------------------------------------------------------
>
> (Updated May 27, 2016, 12:40 a.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1531
> https://issues.apache.org/jira/browse/SQOOP-1531
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Added functions to shell and client to support getting connectors listed by direction
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e
> client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java adf4eb5
> shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java c90fe94
> shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a5f5d4d
> shell/src/main/resources/shell-resource.properties 2c94c58
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
>
> Diff: https://reviews.apache.org/r/47917/diff/
>
>
> Testing
> -------
>
> Added unit tests.
> Both unit and integration tests run successfully.
>
>
> Thanks,
>
> Erzsebet Szilagyi
>
>
Re: Review Request 47917: Sqoop2: Support getting connectors by
Direction
Posted by Abraham Fine <ab...@abrahamfine.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47917/#review135103
-----------------------------------------------------------
Fix it, then Ship it!
client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java (line 214)
<https://reviews.apache.org/r/47917/#comment200113>
what do you think about using assertFalse instead of assertTrue(!...)?
- Abraham Fine
On May 26, 2016, 10:40 p.m., Erzsebet Szilagyi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47917/
> -----------------------------------------------------------
>
> (Updated May 26, 2016, 10:40 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1531
> https://issues.apache.org/jira/browse/SQOOP-1531
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> Added functions to shell and client to support getting connectors listed by direction
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e
> client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java adf4eb5
> shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java c90fe94
> shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a5f5d4d
> shell/src/main/resources/shell-resource.properties 2c94c58
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
>
> Diff: https://reviews.apache.org/r/47917/diff/
>
>
> Testing
> -------
>
> Added unit tests.
> Both unit and integration tests run successfully.
>
>
> Thanks,
>
> Erzsebet Szilagyi
>
>
Re: Review Request 47917: Sqoop2: Support getting connectors by
Direction
Posted by Erzsebet Szilagyi <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47917/
-----------------------------------------------------------
(Updated May 27, 2016, 12:40 a.m.)
Review request for Sqoop.
Changes
-------
Updated diff added.
Bugs: SQOOP-1531
https://issues.apache.org/jira/browse/SQOOP-1531
Repository: sqoop-sqoop2
Description
-------
Added functions to shell and client to support getting connectors listed by direction
Diffs (updated)
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e
client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java adf4eb5
shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java c90fe94
shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a5f5d4d
shell/src/main/resources/shell-resource.properties 2c94c58
shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
Diff: https://reviews.apache.org/r/47917/diff/
Testing
-------
Added unit tests.
Both unit and integration tests run successfully.
Thanks,
Erzsebet Szilagyi
Re: Review Request 47917: Sqoop2: Support getting connectors by
Direction
Posted by Erzsebet Szilagyi <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47917/
-----------------------------------------------------------
(Updated May 27, 2016, 12:35 a.m.)
Review request for Sqoop.
Bugs: SQOOP-1531
https://issues.apache.org/jira/browse/SQOOP-1531
Repository: sqoop-sqoop2
Description
-------
Added functions to shell and client to support getting connectors listed by direction
Diffs (updated)
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e
client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java adf4eb5
shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java c90fe94
shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a5f5d4d
shell/src/main/resources/shell-resource.properties 2c94c58
shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
Diff: https://reviews.apache.org/r/47917/diff/
Testing
-------
Added unit tests.
Both unit and integration tests run successfully.
Thanks,
Erzsebet Szilagyi
Re: Review Request 47917: Sqoop2: Support getting connectors by
Direction
Posted by Erzsebet Szilagyi <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47917/
-----------------------------------------------------------
(Updated May 26, 2016, 11:46 p.m.)
Review request for Sqoop.
Bugs: SQOOP-1531
https://issues.apache.org/jira/browse/SQOOP-1531
Repository: sqoop-sqoop2
Description
-------
Added functions to shell and client to support getting connectors listed by direction
Diffs
-----
client/src/main/java/org/apache/sqoop/client/SqoopClient.java 1cf549e
client/src/test/java/org/apache/sqoop/client/TestSqoopClient.java adf4eb5
shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java c90fe94
shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a5f5d4d
shell/src/main/resources/shell-resource.properties 2c94c58
shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
Diff: https://reviews.apache.org/r/47917/diff/
Testing
-------
Added unit tests.
Both unit and integration tests run successfully.
Thanks,
Erzsebet Szilagyi