You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Jarek Cecho <ja...@apache.org> on 2014/08/07 02:19:15 UTC
Re: Review Request 23944: Sqoop2: From/To: Refactor models
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23944/#review49839
-----------------------------------------------------------
Ship it!
Good work, couple of comments (can be addressed in follow up JIRAs)
common/src/main/java/org/apache/sqoop/model/MJob.java
<https://reviews.apache.org/r/23944/#comment87228>
Since we have only two types - FROM and TO, I would suggest to using variable references rather then map here. I'm assuming that direct reference will be faster then map lookup and also it might help with readability of the code as we're expecting that the map will have always exactly 2 items. I would suggest to do the change in follow-up JIRA though.
common/src/main/java/org/apache/sqoop/model/MJobForms.java
<https://reviews.apache.org/r/23944/#comment87229>
It seems that this class is pretty much useless at this point as it's just wrapping the parent class, so what about removing it in followup JIRA?
Jarcec
- Jarek Cecho
On July 25, 2014, 8:08 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23944/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 8:08 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1376
> https://issues.apache.org/jira/browse/SQOOP-1376
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> - Removed Job.Type and created ConnectorType.FROM/TO (not included in Review).
> - MConnector is the only model which has two sets of forms.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/model/MConnector.java 43fad27
> common/src/main/java/org/apache/sqoop/model/MFramework.java c742459
> common/src/main/java/org/apache/sqoop/model/MJob.java 849168d
> common/src/main/java/org/apache/sqoop/model/MJobForms.java f697023
>
> Diff: https://reviews.apache.org/r/23944/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Abraham Elmahrek
>
>
Re: Review Request 23944: Sqoop2: From/To: Refactor models
Posted by Abraham Elmahrek <ab...@cloudera.com>.
> On Aug. 7, 2014, 12:19 a.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/model/MJob.java, lines 43-48
> > <https://reviews.apache.org/r/23944/diff/1/?file=642277#file642277line43>
> >
> > Since we have only two types - FROM and TO, I would suggest to using variable references rather then map here. I'm assuming that direct reference will be faster then map lookup and also it might help with readability of the code as we're expecting that the map will have always exactly 2 items. I would suggest to do the change in follow-up JIRA though.
https://issues.apache.org/jira/browse/SQOOP-1416
> On Aug. 7, 2014, 12:19 a.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/model/MJobForms.java, line 28
> > <https://reviews.apache.org/r/23944/diff/1/?file=642278#file642278line28>
> >
> > It seems that this class is pretty much useless at this point as it's just wrapping the parent class, so what about removing it in followup JIRA?
https://issues.apache.org/jira/browse/SQOOP-1418
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23944/#review49839
-----------------------------------------------------------
On July 25, 2014, 8:08 p.m., Abraham Elmahrek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23944/
> -----------------------------------------------------------
>
> (Updated July 25, 2014, 8:08 p.m.)
>
>
> Review request for Sqoop.
>
>
> Bugs: SQOOP-1376
> https://issues.apache.org/jira/browse/SQOOP-1376
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> - Removed Job.Type and created ConnectorType.FROM/TO (not included in Review).
> - MConnector is the only model which has two sets of forms.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/sqoop/model/MConnector.java 43fad27
> common/src/main/java/org/apache/sqoop/model/MFramework.java c742459
> common/src/main/java/org/apache/sqoop/model/MJob.java 849168d
> common/src/main/java/org/apache/sqoop/model/MJobForms.java f697023
>
> Diff: https://reviews.apache.org/r/23944/diff/
>
>
> Testing
> -------
>
> N/A
>
>
> Thanks,
>
> Abraham Elmahrek
>
>