sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 23944: Sqoop2: From/To: Refactor models
Date Thu, 07 Aug 2014 00:19:15 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message