----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9717/#review17654 -----------------------------------------------------------
Hi Jarcec, Overall looks great! Thank you very much for working on this. My major comment is why the "io" can't be completely hidden from the Function and Command classes. The "io" is now moved into ShellEnvironment, so I think it shouldn't be visible at all in these classes if possible. For example, I found it odd to see the following code: println(Constants.RES_CLONE_USAGE, getUsage()); io.out.println(); In addition, some methods in the Functions and Command classes take "io" as a parameter while some don't, which I found inconsistent. Besides, I made a few minor comments inline. client/src/main/java/org/apache/sqoop/client/SqoopClient.java <https://reviews.apache.org/r/9717/#comment37468> Wouldn't "getSubmissionStatus" be a better name for this method? client/src/main/java/org/apache/sqoop/client/request/SqoopRequests.java <https://reviews.apache.org/r/9717/#comment37472> Can you factor out this into a separate method so that duplicate code can be removed in createConnectionApplyValidations and updateConnectionApplyValidations? client/src/main/java/org/apache/sqoop/client/request/SqoopRequests.java <https://reviews.apache.org/r/9717/#comment37473> The same here as above. Can you factor out this to remove duplicate code? client/src/main/java/org/apache/sqoop/client/shell/CloneCommand.java <https://reviews.apache.org/r/9717/#comment37475> Shouldn't it be "SqoopException(ClientError.CLIENT_0002, usageMsg())"? client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java <https://reviews.apache.org/r/9717/#comment37493> Can't you hide io into ShellEnvironment? How about adding a method called errorIntroduction() to ShellEnvironment and call FormFiller.errorIntroduction(io) inside that method? I just find it inconsistent that you pass io to some methods while you don't to others. I would prefer hiding io completely if possible. client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java <https://reviews.apache.org/r/9717/#comment37494> The same as above. client/src/main/java/org/apache/sqoop/client/shell/CreateCommand.java <https://reviews.apache.org/r/9717/#comment37476> "ShellEnvironment." is not needed. client/src/main/java/org/apache/sqoop/client/shell/CreateConnectionFunction.java <https://reviews.apache.org/r/9717/#comment37477> Can't it be be simplified to "println(Constants.RES_ARGS_CID_MISSING)"? client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java <https://reviews.apache.org/r/9717/#comment37498> "ShellEnvironment." is not needed. client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java <https://reviews.apache.org/r/9717/#comment37499> "ShellEnvironment." is not needed. client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java <https://reviews.apache.org/r/9717/#comment37500> "ShellEnvironment." is not needed. client/src/main/java/org/apache/sqoop/client/shell/ShellEnvironment.java <https://reviews.apache.org/r/9717/#comment37474> Why can't this be simplified as follows? private static String serverHost = "localhost"; private static String serverPort = "12000"; private static String serverWebapp = "sqoop"; private static boolean verbose = false; private static boolean interactive; static ResourceBundle resource = ResourceBundle.getBundle(Constants.RESOURCE_NAME, Locale.getDefault());; static SqoopClient client = new SqoopClient(getServerUrl());; client/src/main/java/org/apache/sqoop/client/shell/ShowOptionFunction.java <https://reviews.apache.org/r/9717/#comment37501> "ShellEnvironment." is not needed. client/src/main/java/org/apache/sqoop/client/shell/ShowVersionFunction.java <https://reviews.apache.org/r/9717/#comment37502> "ShellEnvironment." is not needed. client/src/main/java/org/apache/sqoop/client/shell/UpdateCommand.java <https://reviews.apache.org/r/9717/#comment37503> "ShellEnvironment." is not needed. - Cheolsoo Park On March 3, 2013, 2:13 a.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9717/ > ----------------------------------------------------------- > > (Updated March 3, 2013, 2:13 a.m.) > > > Review request for Sqoop. > > > Description > ------- > > I've created first simple SqoopClient API and altered shell to use it. I've > noticed that there is a lot of duplicate code in the shell, so I've tried to > clean it up. The changes are simple, however are spanning across all shell > files, thus making this patch bigger than necessary. > > This is first implementation of the SqoopClient API. I'm intending to improve > it, for example by caching objects that was already received (especially > connector info and resource bundles). As this patch is big enough already, I > would prefer to finish this functionality in follow up JIRA. > > > This addresses bug SQOOP-918. > https://issues.apache.org/jira/browse/SQOOP-918 > > > Diffs > ----- > > client/src/main/java/org/apache/sqoop/client/SqoopClient.java PRE-CREATION > client/src/main/java/org/apache/sqoop/client/core/Environment.java > 5d1af26a0e8f96c11c7ef7d8fcfd0013db5ccce6 > client/src/main/java/org/apache/sqoop/client/core/RequestCache.java > 808b9f15d52ba9d3620e094a3640b6a93e2b0848 > client/src/main/java/org/apache/sqoop/client/request/ConnectionRequest.java > f1e4d685eb0466e916ede21e23f6766f6b263c27 > client/src/main/java/org/apache/sqoop/client/request/ConnectorRequest.java > 9ea9d5db91cb28337ef4b6d997af9d0cd0ef7781 > client/src/main/java/org/apache/sqoop/client/request/JobRequest.java > c2449f5106e2918b5e5068ff41733def68180804 > client/src/main/java/org/apache/sqoop/client/request/SqoopRequests.java > PRE-CREATION > client/src/main/java/org/apache/sqoop/client/request/SubmissionRequest.java > 60dcbb29cb75106c58d4de29a6160c1976bc4d67 > client/src/main/java/org/apache/sqoop/client/shell/CloneCommand.java > abec66b4bdcd10bfe6a7b62e421a131083359b5e > > client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java > 9d6c39627fc7c244adfc6a23a0d6470ab834ee9e > client/src/main/java/org/apache/sqoop/client/shell/CloneJobFunction.java > d06f05113cee801e0d956aa2f07f12f6bf748c46 > client/src/main/java/org/apache/sqoop/client/shell/CreateCommand.java > f4872d2822859739d0bf86c32d349190905d52ad > > client/src/main/java/org/apache/sqoop/client/shell/CreateConnectionFunction.java > 2c750d337a34a6e2c08f524dfa61a3c287f385d1 > client/src/main/java/org/apache/sqoop/client/shell/CreateJobFunction.java > 1f01cc75b5be572385c8ad3c6c20e33903e88fd6 > client/src/main/java/org/apache/sqoop/client/shell/DeleteCommand.java > 38b2fdade23a046e6f4e5c5fcfb823d88181db02 > > client/src/main/java/org/apache/sqoop/client/shell/DeleteConnectionFunction.java > 154acc7382335773f65569c045d7a4dff02eb728 > client/src/main/java/org/apache/sqoop/client/shell/DeleteJobFunction.java > 9d6c53e7fcf4e43753c20668697bdfb25168b927 > client/src/main/java/org/apache/sqoop/client/shell/HelpCommand.java > e14f738d5fd77ed7f7ec913cae2456aa4edd2123 > client/src/main/java/org/apache/sqoop/client/shell/SetCommand.java > 6c17e2515c6c034feb6569b6f8b40e53504b6412 > client/src/main/java/org/apache/sqoop/client/shell/SetOptionFunction.java > 5d5e9e7570e432c2e8c00c656153a2f1c09bb14c > client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java > 0e1e027c04829cb9e00fc307d182dccc9c119fcf > client/src/main/java/org/apache/sqoop/client/shell/ShellEnvironment.java > PRE-CREATION > client/src/main/java/org/apache/sqoop/client/shell/ShowCommand.java > a043aa339b8617e17216627c425362f305e9ac1b > > client/src/main/java/org/apache/sqoop/client/shell/ShowConnectionFunction.java > af325d5431fa9ab3cc1cc1d58cdd99f393c84c50 > > client/src/main/java/org/apache/sqoop/client/shell/ShowConnectorFunction.java > c2464d2c16bf5573c21e23b470dc074826a15468 > > client/src/main/java/org/apache/sqoop/client/shell/ShowFrameworkFunction.java > 31de8dc6d0d5126188f796dafc026934c83237e6 > client/src/main/java/org/apache/sqoop/client/shell/ShowJobFunction.java > 0d67133c30b14755b7d47a30964b18a96e6a908a > client/src/main/java/org/apache/sqoop/client/shell/ShowOptionFunction.java > d8af0b22660f4367aa80dd22f4fdb1fc97c84fe0 > client/src/main/java/org/apache/sqoop/client/shell/ShowServerFunction.java > e1f6fa66cdfb99934b331eae32c5abc97550af67 > client/src/main/java/org/apache/sqoop/client/shell/ShowVersionFunction.java > 0dfc6c842acb1081c9c3d8c028a2a8e9be71e018 > client/src/main/java/org/apache/sqoop/client/shell/SqoopCommand.java > df9350f9680101269f57fbd52cd1e4f783f9dc1d > client/src/main/java/org/apache/sqoop/client/shell/SqoopFunction.java > 5632baca98ee9b4e883fec3a509a04186b0142c8 > client/src/main/java/org/apache/sqoop/client/shell/SqoopShell.java > b4d352c105ebe9e3f6806731e9246d50918b1409 > client/src/main/java/org/apache/sqoop/client/shell/SubmissionCommand.java > af4231d583da428d9c851163bb8d5822887c6b7f > > client/src/main/java/org/apache/sqoop/client/shell/SubmissionStartFunction.java > ba05dbbf8e21e1a4349e7a7f9f978ef5599452f1 > > client/src/main/java/org/apache/sqoop/client/shell/SubmissionStatusFunction.java > dd63cd1f594efb633e62aec2daa0c38b0b51288e > > client/src/main/java/org/apache/sqoop/client/shell/SubmissionStopFunction.java > e71f8bf8d18672c54cf8e512bc46d6e25732172b > client/src/main/java/org/apache/sqoop/client/shell/UpdateCommand.java > 0057efbbea8b2a3ac63bcaf58406b577918e0837 > > client/src/main/java/org/apache/sqoop/client/shell/UpdateConnectionFunction.java > dd0df24e45454abf570560a7ef7a59aa4a650a8c > client/src/main/java/org/apache/sqoop/client/shell/UpdateJobFunction.java > 4fac97726fad4a91380007f8512ab09785042b51 > client/src/main/java/org/apache/sqoop/client/utils/FormFiller.java > e3fe02f91e5bf185323d928151f79f360581a20d > client/src/main/java/org/apache/sqoop/client/utils/SubmissionDisplayer.java > 8afb4926dd6cf01f1a353f0cbb451d121e4b6c90 > client/src/main/java/org/apache/sqoop/client/utils/ThrowableDisplayer.java > 45c78fb26afedacec47074f7c7a714a77df45ec3 > test/src/test/java/org/apache/sqoop/integration/TomcatTestCase.java > 8b4617954cbc952bfb4739e91339ad475fc5a737 > > test/src/test/java/org/apache/sqoop/integration/connector/jdbc/generic/TableImportTest.java > 20588d31da48ac93d979718c22fb4e179d6b2d19 > > Diff: https://reviews.apache.org/r/9717/diff/ > > > Testing > ------- > > Unit tests seems to be passing and I've tried the changes on real cluster. > > > Thanks, > > Jarek Cecho > >
