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