You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by rj...@gmail.com on 2013/05/01 23:01:53 UTC
Review Request: Sqoop2: Add cloning ability to model classes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10886/
-----------------------------------------------------------
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
Re: Review Request: Sqoop2: Add cloning ability to model classes
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10886/#review20933
-----------------------------------------------------------
Ship it!
Ship It!
- Jarek Cecho
On May 22, 2013, 6:44 p.m., vasanthkumar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10886/
> -----------------------------------------------------------
>
> (Updated May 22, 2013, 6:44 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
>
>
Re: Review Request: Sqoop2: Add cloning ability to model classes
Posted by rj...@gmail.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10886/
-----------------------------------------------------------
(Updated May 22, 2013, 6:44 p.m.)
Review request for Sqoop.
Changes
-------
Thank you Jarek for your comments and Implemented. Kindly review.
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 (updated)
-----
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
Re: Review Request: Sqoop2: Add cloning ability to model classes
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Sqoop2: Add cloning ability to model classes
Posted by rj...@gmail.com.
-----------------------------------------------------------
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.
Changes
-------
Implemented Jarek's suggestion. Kindly review.
Description (updated)
-------
Cloning Model classes by introduce clone method in model classes.
This addresses bug SQOOP-995.
https://issues.apache.org/jira/browse/SQOOP-995
Diffs (updated)
-----
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)
-------
Updated related test classes.
Thanks,
vasanthkumar
Re: Review Request: Sqoop2: Add cloning ability to model classes
Posted by rj...@gmail.com.
> 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
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
- vasanthkumar
-----------------------------------------------------------
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
>
>
Re: Review Request: Sqoop2: Add cloning ability to model classes
Posted by Jarek Cecho <ja...@apache.org>.
> 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
>
>
Re: Review Request: Sqoop2: Add cloning ability to model classes
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10886/#review20038
-----------------------------------------------------------
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
- Jarek Cecho
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
>
>