> On March 10, 2013, 7:02 a.m., Cheolsoo Park wrote: > > 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. > >
Hi Cheolsoo, thank you very much for your review of this huge beast. Again please accept my apologies for the giant patch. My original goal was more to clean up the code and remove code repetition rather then encapsulate the IO object into ShellEnvironment. The encapsulation was more side effect that intention. Thus I was explicitly passing the io to all classes that are not in the shell package - to all util classes like FormDisplayer, FormFiller, ... . I however do see your reasoning and I agree, so I'll change that. Unfortunately it will make the patch even bigger. > On March 10, 2013, 7:02 a.m., Cheolsoo Park wrote: > > client/src/main/java/org/apache/sqoop/client/request/SqoopRequests.java, > > lines 129-140 > > <https://reviews.apache.org/r/9717/diff/1/?file=264575#file264575line129> > > > > Can you factor out this into a separate method so that duplicate code > > can be removed in createConnectionApplyValidations and > > updateConnectionApplyValidations? > > I would prefer to do it once SQOOP-919 will get committed, so that I can use the newly introduced MFormList list class during refactorization. Would be follow up JIRA acceptable for you at this point? > On March 10, 2013, 7:02 a.m., Cheolsoo Park wrote: > > client/src/main/java/org/apache/sqoop/client/request/SqoopRequests.java, > > lines 179-190 > > <https://reviews.apache.org/r/9717/diff/1/?file=264575#file264575line179> > > > > The same here as above. Can you factor out this to remove duplicate > > code? I would prefer to do it once SQOOP-919 will get committed, so that I can use the newly introduced MFormList list class during refactorization. Would be follow up JIRA acceptable for you at this point? > On March 10, 2013, 7:02 a.m., Cheolsoo Park wrote: > > client/src/main/java/org/apache/sqoop/client/shell/CloneCommand.java, line > > 73 > > <https://reviews.apache.org/r/9717/diff/1/?file=264577#file264577line73> > > > > Shouldn't it be "SqoopException(ClientError.CLIENT_0002, usageMsg())"? Thank you for pint-pointing this sir. I actually believe that we should not throw an exception here as it's not user friendly. I was planning to do that in follow up jira to limit scope of this one that is already quite large. Since you've mentioned it, I'll fix that. > On March 10, 2013, 7:02 a.m., Cheolsoo Park wrote: > > client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java, > > line 108 > > <https://reviews.apache.org/r/9717/diff/1/?file=264578#file264578line108> > > > > 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. Indeed I can and will do. > On March 10, 2013, 7:02 a.m., Cheolsoo Park wrote: > > client/src/main/java/org/apache/sqoop/client/shell/CloneConnectionFunction.java, > > line 112 > > <https://reviews.apache.org/r/9717/diff/1/?file=264578#file264578line112> > > > > The same as above. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9717/#review17654 ----------------------------------------------------------- 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 > >
