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