You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sqoop.apache.org by Veena Basavaraj <vb...@cloudera.com> on 2014/09/12 19:36:05 UTC

Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

Review request for Sqoop.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferrable ( Since the refactoring to From/to it really does not make sense to call them "baseCallbacks", everything in the pluggable/API model is a callback, and from/to are no different)

renamed SubmissionRequest to JobRequest (See JIRA that explains why this seems more intuitive ) , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #2 ( execution engine now creates a job request and a submission request. submission engine submits the job request)... uuf!
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
fixed #3 ( added a unit test for the job manager submit call)

Aside
remove unused imports from the some classes

more clean up
- rename the fallouts from the from/to refactoring to reflect the from/to schema. ( this overlaps with some of the work in https://issues.apache.org/jira/browse/SQOOP-1378 https://reviews.apache.org/r/25180/ from Gwen still pending commit_


Diffs
-----

  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Sept. 13, 2014, 1:01 p.m., Abraham Elmahrek wrote:
> > common/src/main/java/org/apache/sqoop/model/MSubmission.java, line 107
> > <https://reviews.apache.org/r/25586/diff/1/?file=687782#file687782line107>
> >
> >     Can we leave the schema stuff alone for SQOOP-1378?
> 
> Veena Basavaraj wrote:
>     it is simple renamings and not tied to the functionality of supporting matchers.

as discussed with Abe, I will update the JIRA and RB to reflect these renames, that mostly are fallouts from the from/to refactoring.


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53215
-----------------------------------------------------------


On Sept. 12, 2014, 10:42 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2014, 10:42 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferrable ( Since the refactoring to From/to it really does not make sense to call them "baseCallbacks", everything in the pluggable/API model is a callback, and from/to are no different)
> 
> renamed SubmissionRequest to JobRequest (See JIRA that explains why this seems more intuitive ) , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #2 ( execution engine now creates a job request and a submission request. submission engine submits the job request)... uuf!
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> fixed #3 ( added a unit test for the job manager submit call)
> 
> Aside
> remove unused imports from the some classes
> 
> more clean up
> - rename the fallouts from the from/to refactoring to reflect the from/to schema. ( this overlaps with some of the work in https://issues.apache.org/jira/browse/SQOOP-1378 https://reviews.apache.org/r/25180/ from Gwen still pending commit_
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Sept. 13, 2014, 1:01 p.m., Abraham Elmahrek wrote:
> > Went through it once... A few details:
> > 1. I can't seem to find the "Transferrable" class. Is it possible that you have previous commits that are missing?
> > 2. We should rename "Transferrable" to "Transferable".
> > 3. Kill off the extra white space!

fixed all 3


> On Sept. 13, 2014, 1:01 p.m., Abraham Elmahrek wrote:
> > common/src/main/java/org/apache/sqoop/model/MSubmission.java, line 107
> > <https://reviews.apache.org/r/25586/diff/1/?file=687782#file687782line107>
> >
> >     Can we leave the schema stuff alone for SQOOP-1378?

it is simple renamings and not tied to the functionality of supporting matchers.


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53215
-----------------------------------------------------------


On Sept. 12, 2014, 10:42 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2014, 10:42 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferrable ( Since the refactoring to From/to it really does not make sense to call them "baseCallbacks", everything in the pluggable/API model is a callback, and from/to are no different)
> 
> renamed SubmissionRequest to JobRequest (See JIRA that explains why this seems more intuitive ) , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #2 ( execution engine now creates a job request and a submission request. submission engine submits the job request)... uuf!
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> fixed #3 ( added a unit test for the job manager submit call)
> 
> Aside
> remove unused imports from the some classes
> 
> more clean up
> - rename the fallouts from the from/to refactoring to reflect the from/to schema. ( this overlaps with some of the work in https://issues.apache.org/jira/browse/SQOOP-1378 https://reviews.apache.org/r/25180/ from Gwen still pending commit_
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Abraham Elmahrek <ab...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53215
-----------------------------------------------------------


Went through it once... A few details:
1. I can't seem to find the "Transferrable" class. Is it possible that you have previous commits that are missing?
2. We should rename "Transferrable" to "Transferable".
3. Kill off the extra white space!


common/src/main/java/org/apache/sqoop/model/MSubmission.java
<https://reviews.apache.org/r/25586/#comment92713>

    Can we leave the schema stuff alone for SQOOP-1378?



core/src/main/java/org/apache/sqoop/framework/JobManager.java
<https://reviews.apache.org/r/25586/#comment92716>

    Rename to "fromTransferable" and "toTransferable".


- Abraham Elmahrek


On Sept. 12, 2014, 5:42 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2014, 5:42 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferrable ( Since the refactoring to From/to it really does not make sense to call them "baseCallbacks", everything in the pluggable/API model is a callback, and from/to are no different)
> 
> renamed SubmissionRequest to JobRequest (See JIRA that explains why this seems more intuitive ) , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #2 ( execution engine now creates a job request and a submission request. submission engine submits the job request)... uuf!
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> fixed #3 ( added a unit test for the job manager submit call)
> 
> Aside
> remove unused imports from the some classes
> 
> more clean up
> - rename the fallouts from the from/to refactoring to reflect the from/to schema. ( this overlaps with some of the work in https://issues.apache.org/jira/browse/SQOOP-1378 https://reviews.apache.org/r/25180/ from Gwen still pending commit_
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Sept. 16, 2014, 6:28 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/framework/JobManager.java, line 496
> > <https://reviews.apache.org/r/25586/diff/2/?file=690296#file690296line496>
> >
> >     It seems as a difference between from/to.
> 
> Veena Basavaraj wrote:
>     Object fromFrameworkConnection = ClassUtils.instantiate(FrameworkManager.getInstance()
>             .getConnectionConfigurationClass());
>         FormUtils.fromForms(fromConnection.getFrameworkPart().getForms(), fromFrameworkConnection);
>     
>         Object toFrameworkConnection = ClassUtils.instantiate(FrameworkManager.getInstance()
>             .getConnectionConfigurationClass());
>         FormUtils.fromForms(toConnection.getFrameworkPart().getForms(), toFrameworkConnection);
>     
>     
>     So far we do not have the from and to differences for the framework. Hence the question.

Also I saw this comment from Abe in the code and this is related to my question above.

    // TODO(Abe): Should we actually have 2 different Framework Connection config objects?
    jobRequest.setFrameworkConnectionConfig(Direction.FROM, fromFrameworkConnection);
    jobRequest.setFrameworkConnectionConfig(Direction.TO, toFrameworkConnection);


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53517
-----------------------------------------------------------


On Sept. 16, 2014, 1:19 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 1:19 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Sept. 16, 2014, 6:28 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/framework/JobManager.java, line 410
> > <https://reviews.apache.org/r/25586/diff/2/?file=690296#file690296line410>
> >
> >     This is a global lock on Sqoop 2 server level, so we're limiting the critical section to the smallest area possible.
> 
> Veena Basavaraj wrote:
>     I might be missing something, but we end up creating the entire request for submissione etc, my question why not move this code of checing for an exisitng submission even before we prepare the job request?

reverted to original code.


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53517
-----------------------------------------------------------


On Sept. 16, 2014, 7:18 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 7:18 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/common/Direction.java 1576b96 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Sept. 16, 2014, 6:28 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/framework/JobManager.java, line 410
> > <https://reviews.apache.org/r/25586/diff/2/?file=690296#file690296line410>
> >
> >     This is a global lock on Sqoop 2 server level, so we're limiting the critical section to the smallest area possible.

I might be missing something, but we end up creating the entire request for submissione etc, my question why not move this code of checing for an exisitng submission even before we prepare the job request?


> On Sept. 16, 2014, 6:28 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/framework/JobManager.java, line 496
> > <https://reviews.apache.org/r/25586/diff/2/?file=690296#file690296line496>
> >
> >     It seems as a difference between from/to.

Object fromFrameworkConnection = ClassUtils.instantiate(FrameworkManager.getInstance()
        .getConnectionConfigurationClass());
    FormUtils.fromForms(fromConnection.getFrameworkPart().getForms(), fromFrameworkConnection);

    Object toFrameworkConnection = ClassUtils.instantiate(FrameworkManager.getInstance()
        .getConnectionConfigurationClass());
    FormUtils.fromForms(toConnection.getFrameworkPart().getForms(), toFrameworkConnection);


So far we do not have the from and to differences for the framework. Hence the question.


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53517
-----------------------------------------------------------


On Sept. 16, 2014, 1:19 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 1:19 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Jarek Cecho <ja...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53517
-----------------------------------------------------------



core/src/main/java/org/apache/sqoop/framework/JobManager.java
<https://reviews.apache.org/r/25586/#comment93205>

    This is a global lock on Sqoop 2 server level, so we're limiting the critical section to the smallest area possible.



core/src/main/java/org/apache/sqoop/framework/JobManager.java
<https://reviews.apache.org/r/25586/#comment93206>

    It seems as a difference between from/to.


- Jarek Cecho


On Sept. 16, 2014, 8:19 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 8:19 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Sept. 17, 2014, 11:42 a.m., Abraham Elmahrek wrote:
> > Couple of notes:
> > 1. Can we move the TestJdbcRepository changes to a different Jira?
> > 2. Watch out for extra spaces.
> > 3. Try to keep changes that aren't local to this Jira out of the review.

all it does it renames the variables to suffix with mock to make it more readable, since it is east to infer what is mocked Vs what is stubbed and Vs what is tested,. I dont think I have modified any tests


> On Sept. 17, 2014, 11:42 a.m., Abraham Elmahrek wrote:
> > common/src/main/java/org/apache/sqoop/common/Direction.java, line 30
> > <https://reviews.apache.org/r/25586/diff/3/?file=691383#file691383line30>
> >
> >     Where is this used?

can you use the latest rev. I removed it, I added it for testing something and did not think it made sense later


> On Sept. 17, 2014, 11:42 a.m., Abraham Elmahrek wrote:
> > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java, line 103
> > <https://reviews.apache.org/r/25586/diff/3/?file=691392#file691392line103>
> >
> >     This change doesn't seem to serve a direct purpose. Can we nix it?

fixed


> On Sept. 17, 2014, 11:42 a.m., Abraham Elmahrek wrote:
> > execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java, line 238
> > <https://reviews.apache.org/r/25586/diff/4/?file=691486#file691486line238>
> >
> >     This should be setConnectorSchema.

there is schema for from and to. IMO connector is superflous and it is implict. I will add it back if that is a knit pick


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53636
-----------------------------------------------------------


On Sept. 16, 2014, 7:22 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 7:22 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Abraham Elmahrek <ab...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53636
-----------------------------------------------------------


Couple of notes:
1. Can we move the TestJdbcRepository changes to a different Jira?
2. Watch out for extra spaces.
3. Try to keep changes that aren't local to this Jira out of the review.


common/src/main/java/org/apache/sqoop/common/Direction.java
<https://reviews.apache.org/r/25586/#comment93344>

    Where is this used?



core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java
<https://reviews.apache.org/r/25586/#comment93346>

    This change doesn't seem to serve a direct purpose. Can we nix it?



core/src/main/java/org/apache/sqoop/framework/JobManager.java
<https://reviews.apache.org/r/25586/#comment93353>

    getToSchema



execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java
<https://reviews.apache.org/r/25586/#comment93354>

    This should be setConnectorSchema.



execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java
<https://reviews.apache.org/r/25586/#comment93355>

    This should be getConnectorSchema.


- Abraham Elmahrek


On Sept. 17, 2014, 2:22 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 2:22 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Abraham Elmahrek <ab...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53856
-----------------------------------------------------------


Almost there I think!


common/src/main/java/org/apache/sqoop/json/SubmissionBean.java
<https://reviews.apache.org/r/25586/#comment93720>

    from-schema
    to-schema


- Abraham Elmahrek


On Sept. 17, 2014, 6:56 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 6:56 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Abraham Elmahrek <ab...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review54046
-----------------------------------------------------------

Ship it!


We can handle some of the more nit-picky changes in follow up Jiras. Thanks for working on this Veena!

- Abraham Elmahrek


On Sept. 17, 2014, 6:56 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 6:56 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 17, 2014, 11:56 a.m.)


Review request for Sqoop.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferable
renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)

fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)

fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)

Misc cleanup

-Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
-Removed some unused imports in some classes
-Renamed the connecto/hio to from/to schema in the submission class
- Added a comment on a commented out tests for the reason it is commented out


Diffs (updated)
-----

  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
  core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
  spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 16, 2014, 7:22 p.m.)


Review request for Sqoop.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferable
renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)

fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)

fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)

Misc cleanup

-Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
-Removed some unused imports in some classes
-Renamed the connecto/hio to from/to schema in the submission class
- Added a comment on a commented out tests for the reason it is commented out


Diffs (updated)
-----

  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
  core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
  server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
  spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 16, 2014, 7:18 p.m.)


Review request for Sqoop.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferable
renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)

fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)

fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)

Misc cleanup

-Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
-Removed some unused imports in some classes
-Renamed the connecto/hio to from/to schema in the submission class
- Added a comment on a commented out tests for the reason it is commented out


Diffs (updated)
-----

  common/src/main/java/org/apache/sqoop/common/Direction.java 1576b96 
  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
  core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
  server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
  spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 16, 2014, 6:35 p.m.)


Review request for Sqoop.


Changes
-------

minor- remove whitespaces.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferable
renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)

fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)

fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)

Misc cleanup

-Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
-Removed some unused imports in some classes
-Renamed the connecto/hio to from/to schema in the submission class
- Added a comment on a commented out tests for the reason it is commented out


Diffs (updated)
-----

  common/src/main/java/org/apache/sqoop/common/Direction.java 1576b96 
  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
  core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
  server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
  spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 16, 2014, 6:29 p.m.)


Review request for Sqoop.


Changes
-------

Add #3, job manager tests


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferable
renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)

fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)

fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)

Misc cleanup

-Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
-Removed some unused imports in some classes
-Renamed the connecto/hio to from/to schema in the submission class
- Added a comment on a commented out tests for the reason it is commented out


Diffs (updated)
-----

  common/src/main/java/org/apache/sqoop/common/Direction.java 1576b96 
  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
  core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
  server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
  spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Sept. 16, 2014, 3:01 p.m., Abraham Elmahrek wrote:
> > core/src/main/java/org/apache/sqoop/framework/JobManager.java, lines 296-297
> > <https://reviews.apache.org/r/25586/diff/2/?file=690296#file690296line296>
> >
> >     This should be part of the synchronized block. Otherwise, there are cases where we check if a submission is started, but start the submission after another thread starts a submission for the same job.

yes, this is fixed, The checking if the last submission is running and then submitting are in the synchronized  block as before. no changes


> On Sept. 16, 2014, 3:01 p.m., Abraham Elmahrek wrote:
> > execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java, lines 89-95
> > <https://reviews.apache.org/r/25586/diff/2/?file=690306#file690306line89>
> >
> >     This seems like duplicate logic. Let's yank it?

Nice catch:)


> On Sept. 16, 2014, 3:01 p.m., Abraham Elmahrek wrote:
> > core/src/main/java/org/apache/sqoop/framework/JobManager.java, line 426
> > <https://reviews.apache.org/r/25586/diff/2/?file=690296#file690296line426>
> >
> >     How about renaming this method or breaking it up? This method does a bit more than just get the schema. Maybe something like "setupConnector".

split into multiple methods yes


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53602
-----------------------------------------------------------


On Sept. 16, 2014, 6:35 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 6:35 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/common/Direction.java 1576b96 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java db6f579 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java 50daa62 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   server/src/main/java/org/apache/sqoop/handler/ConnectorRequestHandler.java d1b6b9a 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Abraham Elmahrek <ab...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/#review53602
-----------------------------------------------------------


Chipping away at this review. A few more details below.


common/src/main/java/org/apache/sqoop/model/MSubmission.java
<https://reviews.apache.org/r/25586/#comment93319>

    upper case FROM and TO? Just to discern between the words "from" and "to". Makes it stand out a bit more.



core/src/main/java/org/apache/sqoop/framework/JobManager.java
<https://reviews.apache.org/r/25586/#comment93309>

    This should be part of the synchronized block. Otherwise, there are cases where we check if a submission is started, but start the submission after another thread starts a submission for the same job.



core/src/main/java/org/apache/sqoop/framework/JobManager.java
<https://reviews.apache.org/r/25586/#comment93318>

    How about renaming this method or breaking it up? This method does a bit more than just get the schema. Maybe something like "setupConnector".



execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java
<https://reviews.apache.org/r/25586/#comment93310>

    Typo "claRsses".



execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java
<https://reviews.apache.org/r/25586/#comment93314>

    This seems like duplicate logic. Let's yank it?


- Abraham Elmahrek


On Sept. 16, 2014, 8:19 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25586/
> -----------------------------------------------------------
> 
> (Updated Sept. 16, 2014, 8:19 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1496
>     https://issues.apache.org/jira/browse/SQOOP-1496
> 
> 
> Repository: sqoop-SQOOP-1367
> 
> 
> Description
> -------
> 
> renamed BaseCallbacks to Transferable
> renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
> fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
> 
> fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)
> 
> fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)
> 
> Misc cleanup
> 
> -Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
> -Removed some unused imports in some classes
> -Renamed the connecto/hio to from/to schema in the submission class
> - Added a comment on a commented out tests for the reason it is commented out
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
>   common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
>   common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
>   connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
>   connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
>   connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
>   core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
>   core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
>   core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
>   core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
>   core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
>   core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
>   execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
>   execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
>   execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
>   shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
>   spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
>   spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
>   spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
>   spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
>   spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
>   submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 
> 
> Diff: https://reviews.apache.org/r/25586/diff/
> 
> 
> Testing
> -------
> 
> unit/ integration tests pass
> 
> if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look
> 
> 
> Thanks,
> 
> Veena Basavaraj
> 
>

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 16, 2014, 1:19 a.m.)


Review request for Sqoop.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferable
renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)

fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)

fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)

Misc cleanup

-Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
-Removed some unused imports in some classes
-Renamed the connecto/hio to from/to schema in the submission class
- Added a comment on a commented out tests for the reason it is commented out


Diffs
-----

  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
  core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
  spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 15, 2014, 7:01 p.m.)


Review request for Sqoop.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description (updated)
-------

renamed BaseCallbacks to Transferable
renamed SubmissionRequest to JobRequest , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)

fixed #2 ( execution engine now creates a job request instead of a submission request. submission engine submits the job request)

fixed #3 ( added a unit test for the job manager, one of the most important class that has no tests)

Misc cleanup

-Renamed the CSVIntermediateDataFormat test class to start with Test ( another knitpick to remain consistent with naming classes)
-Removed some unused imports in some classes
-Renamed the connecto/hio to from/to schema in the submission class
- Added a comment on a commented out tests for the reason it is commented out


Diffs (updated)
-----

  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java eb6fcf1 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/TestCSVIntermediateDataFormat.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/JobRequest.java PRE-CREATION 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestFrameworkValidator.java 69c1b56 
  core/src/test/java/org/apache/sqoop/framework/TestJobManager.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestJobRequest.java PRE-CREATION 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRJobRequest.java PRE-CREATION 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ConfigurationUtils.java 92414d8 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/ProgressRunnable.java b73b151 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopDestroyerExecutor.java 59431f4 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopFileOutputFormat.java bbf7342 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopInputFormat.java 1c1133a 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopMapper.java 3065680 
  execution/mapreduce/src/main/java/org/apache/sqoop/job/mr/SqoopOutputFormatLoadExecutor.java e457cff 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/TestMapReduce.java 2dfc487 
  execution/mapreduce/src/test/java/org/apache/sqoop/job/mr/TestConfigurationUtils.java 1447e00 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  spi/src/main/java/org/apache/sqoop/job/etl/Transferable.java PRE-CREATION 
  spi/src/main/java/org/apache/sqoop/validation/Validator.java 9b791f8 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


File Attachments (updated)
----------------

SQOOP-1496.patch
  https://reviews.apache.org/media/uploaded/files/2014/09/16/d0dc479f-f997-4669-babd-1fdf88a3d17b__SQOOP-1496.patch


Thanks,

Veena Basavaraj

Re: Review Request 25586: SQOOP-1496:Cleanup the SubmissionEngine/ExecutionEngine APIs

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25586/
-----------------------------------------------------------

(Updated Sept. 12, 2014, 10:42 a.m.)


Review request for Sqoop.


Bugs: SQOOP-1496
    https://issues.apache.org/jira/browse/SQOOP-1496


Repository: sqoop-SQOOP-1367


Description
-------

renamed BaseCallbacks to Transferrable ( Since the refactoring to From/to it really does not make sense to call them "baseCallbacks", everything in the pluggable/API model is a callback, and from/to are no different)

renamed SubmissionRequest to JobRequest (See JIRA that explains why this seems more intuitive ) , I am keeping the SubmissionEngine terminology as it is, and will revisit at a later point when a new submission/execution engine will be supported
fixed #2 ( execution engine now creates a job request and a submission request. submission engine submits the job request)... uuf!
fixed #1 ( refactored to smaller methods, so that the code is readable and testable)
fixed #3 ( added a unit test for the job manager submit call)

Aside
remove unused imports from the some classes

more clean up
- rename the fallouts from the from/to refactoring to reflect the from/to schema. ( this overlaps with some of the work in https://issues.apache.org/jira/browse/SQOOP-1378 https://reviews.apache.org/r/25180/ from Gwen still pending commit_


Diffs
-----

  common/src/main/java/org/apache/sqoop/json/SubmissionBean.java 61d6576 
  common/src/main/java/org/apache/sqoop/model/MSubmission.java 1edd6ee 
  common/src/test/java/org/apache/sqoop/json/TestSubmissionBean.java d87655e 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormat.java 1e8ab52 
  connector/connector-sdk/src/main/java/org/apache/sqoop/connector/idf/IntermediateDataFormat.java 91b594e 
  connector/connector-sdk/src/test/java/org/apache/sqoop/connector/idf/CSVIntermediateDataFormatTest.java df6d30f 
  core/src/main/java/org/apache/sqoop/framework/ExecutionEngine.java 96ec148 
  core/src/main/java/org/apache/sqoop/framework/JobManager.java b1b37f6 
  core/src/main/java/org/apache/sqoop/framework/SubmissionEngine.java 3c0f6eb 
  core/src/main/java/org/apache/sqoop/framework/SubmissionRequest.java bf3f785 
  core/src/test/java/org/apache/sqoop/framework/TestSubmissionRequest.java 3078ed2 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MRSubmissionRequest.java 32d598c 
  execution/mapreduce/src/main/java/org/apache/sqoop/execution/mapreduce/MapreduceExecutionEngine.java b05954b 
  shell/src/main/java/org/apache/sqoop/shell/utils/SubmissionDisplayer.java 6dbd870 
  spi/src/main/java/org/apache/sqoop/job/etl/CallbackBase.java 59a9457 
  spi/src/main/java/org/apache/sqoop/job/etl/From.java 9b8d76f 
  spi/src/main/java/org/apache/sqoop/job/etl/To.java a791945 
  submission/mapreduce/src/main/java/org/apache/sqoop/submission/mapreduce/MapreduceSubmissionEngine.java bfa6958 

Diff: https://reviews.apache.org/r/25586/diff/


Testing (updated)
-------

unit/ integration tests pass

if this patch is hard to follow, since I have split the jobmanager submit into like many smaller methods, please feel free to apply the patch on a branch and take a look


Thanks,

Veena Basavaraj