sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request: SQOOP-918 Sqoop2: Introduce client API and change Sqoop shell to use it
Date Sun, 10 Mar 2013 17:10:45 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message