> On May 30, 2013, 5:23 a.m., Abraham Elmahrek wrote: > > client/src/main/java/org/apache/sqoop/client/shell/ShellEnvironment.java, > > line 111 > > <https://reviews.apache.org/r/11535/diff/1/?file=298453#file298453line111> > > > > Is there a default port number constant? If not, I think it should be > > so that it can be referenced by other methods. > > Mengwei Ding wrote: > At the very first beginning of this file, the default port and webapp are > also set in this way. I checked the references of default port, too. Seems > like it's been referenced in no other place. > > > Jarek Cecho wrote: > Do you think that it would make sense to define a variable with default > value and then just reference it instead of using the getEnv each time? > Something like: > > private static String DEFAULT_SERVER_HOST = getEnv(Constants.ENV_HOST, > "localhost"); > > ... > > private static String serverHost = DEFAULT_SERVER_HOST; > ... > > public static void setServerUrl(String ustr) { > ... > serverPort = DEFAULT_SERVER_HOST; > ...
Yes, I did have the similar thoughts. The only thing I feel confused about where to put all these default variables to, cause the Constants file contains all string constants. - Mengwei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11535/#review21175 ----------------------------------------------------------- On May 30, 2013, 3:14 a.m., Mengwei Ding wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11535/ > ----------------------------------------------------------- > > (Updated May 30, 2013, 3:14 a.m.) > > > Review request for Sqoop, Jarek Cecho, Hari Shreedharan, and Abraham Elmahrek. > > > Description > ------- > > commit 42577e8b5e2de3c987dfd99d462a4e766f617729 > Author: Mengwei Ding <mengwei.d...@cloudera.com> > Date: Wed May 29 18:04:53 2013 -0700 > > SQOOP-973 add --url option for 'set server', which enables specifying > host, port and webapp in a more consistant way with other applications. For > example, 'set server --url http://localhost:12000/sqoop' > > :100644 100644 8c3c6a4... e0956a0... M > client/src/main/java/org/apache/sqoop/client/core/Constants.java > :100644 100644 1c85592... c2581c3... M > client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java > :100644 100644 f6d1267... 44007d1... M > client/src/main/java/org/apache/sqoop/client/shell/ShellEnvironment.java > :100644 100644 c7a4cb3... 2a5eea4... M > client/src/main/resources/client-resource.properties > > > This addresses bug SQOOP-973. > https://issues.apache.org/jira/browse/SQOOP-973 > > > Diffs > ----- > > client/src/main/java/org/apache/sqoop/client/core/Constants.java 8c3c6a4 > client/src/main/java/org/apache/sqoop/client/shell/SetServerFunction.java > 1c85592 > client/src/main/java/org/apache/sqoop/client/shell/ShellEnvironment.java > f6d1267 > client/src/main/resources/client-resource.properties c7a4cb3 > > Diff: https://reviews.apache.org/r/11535/diff/ > > > Testing > ------- > > > Thanks, > > Mengwei Ding > >
