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: Sqoop2: Add cloning ability to model classes
Date Fri, 03 May 2013 05:14:22 GMT


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


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