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