----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10886/#review20763 -----------------------------------------------------------
Hi Vasanth, thank you very much for incorporating my suggestions. Greatly appreciated. I do have couple of additional notes (mostly just nits). common/src/main/java/org/apache/sqoop/model/MConnection.java <https://reviews.apache.org/r/10886/#comment42853> Connection/Job id should be moved over only if cloneWithValue is set as well, right? common/src/main/java/org/apache/sqoop/model/MConnector.java <https://reviews.apache.org/r/10886/#comment42854> Can we make here if statement checking that cloneWithValue is false? Connector should never have any values filled and thus it shouldn't make sense from user perspective to require cloning them. common/src/main/java/org/apache/sqoop/model/MFramework.java <https://reviews.apache.org/r/10886/#comment42859> Similarly as in MConnector case, framework should never had any values filled and thus it should not be allowed to copy with values. common/src/main/java/org/apache/sqoop/model/MJob.java <https://reviews.apache.org/r/10886/#comment42858> Similarly as in MConnection case, I think that the job id is part of a "value" in case of job. common/src/test/java/org/apache/sqoop/model/TestMConnection.java <https://reviews.apache.org/r/10886/#comment42856> Can we put the clone method tests into separate test method? This note also applies to all other tests. common/src/test/java/org/apache/sqoop/model/TestMConnection.java <https://reviews.apache.org/r/10886/#comment42857> Can we also assert that connection is the same (assertEquals) for clone1 and clone2? common/src/test/java/org/apache/sqoop/model/TestMConnection.java <https://reviews.apache.org/r/10886/#comment42855> Nit: s/Inputs/Input/ - Jarek Cecho On May 6, 2013, 6:38 p.m., vasanthkumar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10886/ > ----------------------------------------------------------- > > (Updated May 6, 2013, 6:38 p.m.) > > > Review request for Sqoop. > > > Description > ------- > > Cloning Model classes by introduce clone method in model classes. > > > This addresses bug SQOOP-995. > https://issues.apache.org/jira/browse/SQOOP-995 > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/MClonable.java PRE-CREATION > common/src/main/java/org/apache/sqoop/model/MConnection.java c31eafd > common/src/main/java/org/apache/sqoop/model/MConnectionForms.java d289a02 > common/src/main/java/org/apache/sqoop/model/MConnector.java 9207c62 > common/src/main/java/org/apache/sqoop/model/MEnumInput.java 12705a6 > common/src/main/java/org/apache/sqoop/model/MForm.java 76998dd > common/src/main/java/org/apache/sqoop/model/MFormList.java 3bf508d > common/src/main/java/org/apache/sqoop/model/MFramework.java f5252ef > common/src/main/java/org/apache/sqoop/model/MInput.java 7d6215f > common/src/main/java/org/apache/sqoop/model/MIntegerInput.java d23ac31 > common/src/main/java/org/apache/sqoop/model/MJob.java 5b50bfd > common/src/main/java/org/apache/sqoop/model/MJobForms.java bce646f > common/src/main/java/org/apache/sqoop/model/MMapInput.java 704c1f8 > common/src/main/java/org/apache/sqoop/model/MStringInput.java a437a25 > common/src/test/java/org/apache/sqoop/model/TestMConnection.java c3469ff > common/src/test/java/org/apache/sqoop/model/TestMConnector.java 716b124 > common/src/test/java/org/apache/sqoop/model/TestMJob.java dbc791e > > Diff: https://reviews.apache.org/r/10886/diff/ > > > Testing > ------- > > Updated related test classes. > > > Thanks, > > vasanthkumar > >
