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

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


Copy link to this message
-
Re: Review Request: SQOOP-918 Sqoop2: Introduce client API and change Sqoop shell to use it
Cheolsoo Park 2013-03-10, 07:02

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