Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Sqoop >> mail # dev >> Review Request:  Sqoop2: Add cloning ability to model classes


+
rj.vasanthkumar@... 2013-05-01, 21:01
+
Jarek Cecho 2013-05-02, 00:19
+
rj.vasanthkumar@... 2013-05-02, 08:59
Copy link to this message
-
Re: Review Request:  Sqoop2: Add cloning ability to model classes


> On May 2, 2013, 12:19 a.m., Jarek Cecho wrote:
> > Hi Vasanth,
> > thank you very much for working on this!
> >
> > I was thinking about the cloneForms() methods and it's usage of "instanceof" operator. What about creating a clone method on each model object and then just call that method instead of quite long if-elif*-else statement? I think that this way we would be able to overcome the need for instanceof. We can make the method parametric to see if user also wants to copy over the values or not.
> >
> > Jarcec
>
> vasanthkumar wrote:
>     Hi Jarcec,
>     In cloneForms method, if-elif*-else statement used only for identifying MInput type. Even if you provide clone method on each model object while copying its Forms which contains multiple MInput type. So requires "instanceof" to identify the MInput type for creating new instance and copying values.
>    
>     For example: while copying/cloning MConnection, contains two MConnectionForms then MConnectionForms contain MForm list. List may have multiple forms with difference type. Here we have to use "instanceof" for identifying the MInput type.
>    
>     Either making Method parametric with constructor (Eg: MConnection(MConnection, boolean copyValue)) or with clone method clone(boolean copyValue) return model object ?
>    
>     Kindly suggest.
>    
>     Thanks,
>     Vasanth kumar

Hi Vasanth,
I would put the clone(boolean value) method to all model objects including MInput<?>. If we will require implementing the method clone(boolen) in abstract MInput, then each concrete implementation will have to create proper cloning method and I believe that in such case we won't need the if-elif*-else statement at all. I'm not a big fan of "copy constructors", so I would personally prefer to expose the cloning interface only using the "clone" method.

Jarcec
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10886/#review20038
-----------------------------------------------------------
On May 1, 2013, 9:01 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10886/
> -----------------------------------------------------------
>
> (Updated May 1, 2013, 9:01 p.m.)
>
>
> Review request for Sqoop.
>
>
> Description
> -------
>
> Cloning class by copy constructor
>
>
> This addresses bug SQOOP-995.
>     https://issues.apache.org/jira/browse/SQOOP-995
>
>
> Diffs
> -----
>
>   common/src/main/java/org/apache/sqoop/model/FormUtils.java 80b10c8
>   common/src/main/java/org/apache/sqoop/model/MConnection.java c31eafd
>   common/src/main/java/org/apache/sqoop/model/MConnector.java 9207c62
>   common/src/main/java/org/apache/sqoop/model/MFramework.java f5252ef
>   common/src/main/java/org/apache/sqoop/model/MJob.java 5b50bfd
>   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/TestMFramework.java a5366ca
>   common/src/test/java/org/apache/sqoop/model/TestMJob.java dbc791e
>
> Diff: https://reviews.apache.org/r/10886/diff/
>
>
> Testing
> -------
>
> Done
>
>
> Thanks,
>
> vasanthkumar
>
>

+
rj.vasanthkumar@... 2013-05-06, 18:38
+
Jarek Cecho 2013-05-20, 13:43
+
rj.vasanthkumar@... 2013-05-22, 18:45
+
Jarek Cecho 2013-05-23, 08:15