----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13181/#review24508 -----------------------------------------------------------
Abe, Thanks for the patch. This looks pretty good. I did a quick initial and partial review - so I could get some feedback since I'll be able to get back to this only on Monday. Please note that most of the comments follow a pattern - so you should be able to fix it in more classes than I have mentioned here. client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java <https://reviews.apache.org/r/13181/#comment48487> Shouldn't isInteractive arg with value false be passed in? client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java <https://reviews.apache.org/r/13181/#comment48488> isInteractive = false to be passed in? client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java <https://reviews.apache.org/r/13181/#comment48489> Ideally, for 2 methods with the same name, you should not have them behave differently - one is setting isInteractive = false and the other sets it to true. client/src/main/java/org/apache/sqoop/client/shell/CreateConnectionFunction.java <https://reviews.apache.org/r/13181/#comment48490> Same as above, they should behave in the same way client/src/main/java/org/apache/sqoop/client/shell/CreateJobFunction.java <https://reviews.apache.org/r/13181/#comment48491> Same as above client/src/main/java/org/apache/sqoop/client/shell/CreateJobFunction.java <https://reviews.apache.org/r/13181/#comment48492> This same method seems to be overridden in all subclasses - is it possible to move this up, and just pass in the options which should be handled? - Hari Shreedharan On Aug. 1, 2013, 12:40 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13181/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2013, 12:40 a.m.) > > > Review request for Sqoop, Hari Shreedharan and Jarek Cecho. > > > Bugs: SQOOP-773 > https://issues.apache.org/jira/browse/SQOOP-773 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit d894ac7b09a18f3fed1fb58bed7554a873fa8630 > Author: Abraham Elmahrek <[email protected]> > Date: Wed Jul 31 14:23:58 2013 -0700 > > SQOOP-773 Sqoop2: Batch execution support for client commands > > Separated options into two groups: fixed and dynamic options. > Fixed options (IE: connector ID) come first and are used to select > what options should be used in dynamic options. Dynamic options > are automatically created based on forms selected from fixed options. > The keys for these options take on the form "<prefix>-<form > name>-<input-name>". > > :100644 100644 0538901... 7f5df34... M > client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java > :100644 100644 6f62813... 32f8c3f... M > client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java > :100644 100644 ac555e1... c842c4d... M > client/src/main/java/org/apache/sqoop/client/shell/CreateCommand.java > :100644 100644 04b240c... f23e479... M > client/src/main/java/org/apache/sqoop/client/shell/CreateConnectionFunction.java > :100644 100644 cc4d546... 7b40645... M > client/src/main/java/org/apache/sqoop/client/shell/CreateJobFunction.java > :100644 100644 18d3a70... 5220d61... M > client/src/main/java/org/apache/sqoop/client/shell/DeleteConnectionFunction.java > :100644 100644 736be20... e82a47b... M > client/src/main/java/org/apache/sqoop/client/shell/DeleteJobFunction.java > :100644 100644 e04292a... f6cd6e3... M > client/src/main/java/org/apache/sqoop/client/shell/DisableConnectionFunction.java > :100644 100644 5962cd2... 6ea9f0c... M > client/src/main/java/org/apache/sqoop/client/shell/DisableJobFunction.java > :100644 100644 ed6dc3c... 094438b... M > client/src/main/java/org/apache/sqoop/client/shell/EnableConnectionFunction.java > :100644 100644 9e4e320... fb75fa8... M > client/src/main/java/org/apache/sqoop/client/shell/EnableJobFunction.java > :100644 100644 e843ede... ba89724... M > client/src/main/java/org/apache/sqoop/client/shell/SetOptionFunction.java > :100644 100644 41fc17a... a241419... M > client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java > :100644 100644 94f92b3... c11b4f3... M > client/src/main/java/org/apache/sqoop/client/shell/ShowConnectionFunction.java > :100644 100644 b053339... 97e4b9d... M > client/src/main/java/org/apache/sqoop/client/shell/ShowConnectorFunction.java > :100644 100644 58b2c6e... 362d981... M > client/src/main/java/org/apache/sqoop/client/shell/ShowFrameworkFunction.java > :100644 100644 97a240b... 9222f50... M > client/src/main/java/org/apache/sqoop/client/shell/ShowJobFunction.java > :100644 100644 81c5612... 39cf3f3... M > client/src/main/java/org/apache/sqoop/client/shell/ShowOptionFunction.java > :100644 100644 110f67e... ef4c907... M > client/src/main/java/org/apache/sqoop/client/shell/ShowServerFunction.java > :100644 100644 666eb7a... 6933965... M > client/src/main/java/org/apache/sqoop/client/shell/ShowSubmissionFunction.java > :100644 100644 8e17f67... ba65a3c... M > client/src/main/java/org/apache/sqoop/client/shell/ShowVersionFunction.java > :100644 100644 bf26761... be06116... M > client/src/main/java/org/apache/sqoop/client/shell/SqoopFunction.java > :100644 100644 2e1c8d3... 7b4f7db... M > client/src/main/java/org/apache/sqoop/client/shell/StartJobFunction.java > :100644 100644 b854a90... a4808a2... M > client/src/main/java/org/apache/sqoop/client/shell/StatusJobFunction.java > :100644 100644 49ab461... 3a4905f... M > client/src/main/java/org/apache/sqoop/client/shell/StopJobFunction.java > :100644 100644 8556e2b... 15cc722... M > client/src/main/java/org/apache/sqoop/client/shell/UpdateConnectionFunction.java > :100644 100644 425a53f... 4edb306... M > client/src/main/java/org/apache/sqoop/client/shell/UpdateJobFunction.java > :100644 100644 2fbf129... 711865d... M > client/src/main/java/org/apache/sqoop/client/utils/FormFiller.java > :000000 100644 0000000... 38e63ba... A > client/src/main/java/org/apache/sqoop/client/utils/FormOptions.java > > > Diffs > ----- > > > client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java > 0538901 > client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java > 6f62813 > client/src/main/java/org/apache/sqoop/client/shell/CreateCommand.java > ac555e1 > > client/src/main/java/org/apache/sqoop/client/shell/CreateConnectionFunction.java > 04b240c > client/src/main/java/org/apache/sqoop/client/shell/CreateJobFunction.java > cc4d546 > > client/src/main/java/org/apache/sqoop/client/shell/DeleteConnectionFunction.java > 18d3a70 > client/src/main/java/org/apache/sqoop/client/shell/DeleteJobFunction.java > 736be20 > > client/src/main/java/org/apache/sqoop/client/shell/DisableConnectionFunction.java > e04292a > client/src/main/java/org/apache/sqoop/client/shell/DisableJobFunction.java > 5962cd2 > > client/src/main/java/org/apache/sqoop/client/shell/EnableConnectionFunction.java > ed6dc3c > client/src/main/java/org/apache/sqoop/client/shell/EnableJobFunction.java > 9e4e320 > client/src/main/java/org/apache/sqoop/client/shell/SetOptionFunction.java > e843ede > client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java > 41fc17a > > client/src/main/java/org/apache/sqoop/client/shell/ShowConnectionFunction.java > 94f92b3 > > client/src/main/java/org/apache/sqoop/client/shell/ShowConnectorFunction.java > b053339 > > client/src/main/java/org/apache/sqoop/client/shell/ShowFrameworkFunction.java > 58b2c6e > client/src/main/java/org/apache/sqoop/client/shell/ShowJobFunction.java > 97a240b > client/src/main/java/org/apache/sqoop/client/shell/ShowOptionFunction.java > 81c5612 > client/src/main/java/org/apache/sqoop/client/shell/ShowServerFunction.java > 110f67e > > client/src/main/java/org/apache/sqoop/client/shell/ShowSubmissionFunction.java > 666eb7a > client/src/main/java/org/apache/sqoop/client/shell/ShowVersionFunction.java > 8e17f67 > client/src/main/java/org/apache/sqoop/client/shell/SqoopFunction.java > bf26761 > client/src/main/java/org/apache/sqoop/client/shell/StartJobFunction.java > 2e1c8d3 > client/src/main/java/org/apache/sqoop/client/shell/StatusJobFunction.java > b854a90 > client/src/main/java/org/apache/sqoop/client/shell/StopJobFunction.java > 49ab461 > > client/src/main/java/org/apache/sqoop/client/shell/UpdateConnectionFunction.java > 8556e2b > client/src/main/java/org/apache/sqoop/client/shell/UpdateJobFunction.java > 425a53f > client/src/main/java/org/apache/sqoop/client/utils/FormFiller.java 2fbf129 > client/src/main/java/org/apache/sqoop/client/utils/FormOptions.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/13181/diff/ > > > Testing > ------- > > Executed the following commands in a file: > > reate connection --cid 1 --name mysql-test --connector-connection-jdbcDriver > com.mysql.jdbc.Driver --connector-connection-connectionString jdbc:mysql://hu\ > e.ent.cloudera.com/test --connector-connection-username root > --connector-connection-password root > show connection > create job --xid 1 --type import --name mysql-import-job1 > --connector-table-tableName test --connector-table-partitionColumn a > --framework-output-storageTy\ > pe 0 --framework-output-outputFormat 0 --framework-output-outputDirectory > /tmp/output > show job > start job --jid 1 > > > Thanks, > > Abraham Elmahrek > >
