Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Sqoop >> mail # dev >> Review Request: SQOOP-918 Sqoop2: Introduce client API and change Sqoop shell to use it


+
Jarek Cecho 2013-03-03, 02:13
+
Jarek Cecho 2013-03-10, 17:20
+
Jarek Cecho 2013-03-11, 00:04
+
Jarek Cecho 2013-03-11, 02:21
+
Cheolsoo Park 2013-03-11, 04:28
+
Cheolsoo Park 2013-03-11, 02:03
+
Jarek Cecho 2013-03-11, 02:23
+
Cheolsoo Park 2013-03-10, 07:02
Copy link to this message
-
Re: Review Request: SQOOP-918 Sqoop2: Introduce client API and change Sqoop shell to use it


> 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:
+
Jarek Cecho 2013-03-10, 15:17
+
Cheolsoo Park 2013-03-10, 22:55
+
Jarek Cecho 2013-03-11, 00:00