> On Sept. 28, 2015, 4:29 p.m., Jarek Cecho wrote: > > My last round of testing on real cluster:
hi Jareck, Thanks for your review, I updated the following things in the latest patch: 1. Load the jar located in the SQOOP_SERVER_EXTRA_LIB. 2. Add the environment check for the Hadoop related jar: (HADOOP_COMMON_HOME && HADOOP_HDFS_HOME && HADOOP_MAPRED_HOME && HADOOP_YARN_HOME) || HADOOP_HOME 3. Check if the server starts successfully, if not, "Sqoop2 server started." won't be displayed. 4. Fix a bug in function is_sqoop_server_running. 5. Add validation for java command. 6. Remove the pid file when stopping the server. > On Sept. 28, 2015, 4:29 p.m., Jarek Cecho wrote: > > dist/src/main/bin/sqoop.sh, lines 77-82 > > <https://reviews.apache.org/r/38589/diff/2/?file=1084669#file1084669line77> > > > > From some reason on my cluster this method was returning "0" if the > > file didn't existed at all. I had to add a "return 1" at the end of the > > method to workaround it. This should be a bug, updated. > On Sept. 28, 2015, 4:29 p.m., Jarek Cecho wrote: > > dist/src/main/bin/sqoop.sh, line 194 > > <https://reviews.apache.org/r/38589/diff/2/?file=1084669#file1084669line194> > > > > This one I'm not clear about, but I'm looking for your thoughts. Should > > we also remove the the pid file when stopping the server? Good suggestion, to get the server status, we can check pid file exist or not, if exist, validate the pid. > On Sept. 28, 2015, 4:29 p.m., Jarek Cecho wrote: > > dist/src/main/bin/sqoop.sh, line 150 > > <https://reviews.apache.org/r/38589/diff/2/?file=1084669#file1084669line150> > > > > Can we add somewhere check that EXEC_JAVA is a valid command? > > > > I got this output of my cluster when "java" is not available on the > > path: > > > > [root@streamsets-1 sqoop-2.0.0-SNAPSHOT-bin-hadoop200]# > > ./bin/sqoop2-server start > > Sqoop home directory: /root/sqoop-2.0.0-SNAPSHOT-bin-hadoop200 > > PID: /tmp/sqoop-root-jetty-server.pid > > Starting the Sqoop2 server... > > Sqoop2 server started. > > [root@streamsets-1 sqoop-2.0.0-SNAPSHOT-bin-hadoop200]# > > /root/sqoop-2.0.0-SNAPSHOT-bin-hadoop200/bin/sqoop.sh: line 167: java: > > command not found > > echo $? > > 0 > > > > Seems very deceiving as the scripts reports "0" (e.g. success) and > > prints out "Sqoop2 server started" even though that the server didn't > > started at all. Add the validation for java command. > On Sept. 28, 2015, 4:29 p.m., Jarek Cecho wrote: > > dist/src/main/bin/sqoop.sh, lines 28-31 > > <https://reviews.apache.org/r/38589/diff/2/?file=1084669#file1084669line28> > > > > Thank you for incorporating my suggestion here! > > > > Might I suggest to be more defensive and verify that those variables > > are defined? I've tried running without them and I got this output: > > > > Sqoop home directory: /root/sqoop-2.0.0-SNAPSHOT-bin-hadoop200 > > PID: /tmp/sqoop-root-jetty-server.pid > > Starting the Sqoop2 server... > > Sqoop2 server started. > > [root@streamsets-1 sqoop-2.0.0-SNAPSHOT-bin-hadoop200]# 0 [main] > > INFO org.apache.sqoop.core.SqoopServer - Initializing Sqoop server. > > 33 [main] INFO org.apache.sqoop.core.PropertiesConfigurationProvider > > - Starting config file poller thread > > Exception in thread "main" java.lang.NoClassDefFoundError: > > org/apache/hadoop/conf/Configuration > > at > > org.apache.sqoop.security.authentication.SimpleAuthenticationHandler.secureLogin(SimpleAuthenticationHandler.java:36) > > at > > org.apache.sqoop.security.AuthenticationManager.initialize(AuthenticationManager.java:98) > > at > > org.apache.sqoop.core.SqoopServer.initialize(SqoopServer.java:54) > > at > > org.apache.sqoop.server.SqoopJettyServer.<init>(SqoopJettyServer.java:50) > > at > > org.apache.sqoop.server.SqoopJettyServer.main(SqoopJettyServer.java:121) > > Caused by: java.lang.ClassNotFoundException: > > org.apache.hadoop.conf.Configuration > > at java.net.URLClassLoader$1.run(URLClassLoader.java:366) > > at java.net.URLClassLoader$1.run(URLClassLoader.java:355) > > at java.security.AccessController.doPrivileged(Native Method) > > at java.net.URLClassLoader.findClass(URLClassLoader.java:354) > > at java.lang.ClassLoader.loadClass(ClassLoader.java:425) > > at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308) > > at java.lang.ClassLoader.loadClass(ClassLoader.java:358) > > ... 5 more > > > > Which is kind of confusing - the scripts exists with success even > > though that we immediately fail. Add 2 checks: 1. for the environment variables, Hadoop lib related should be set. 2. Start the server, wait 5 seconds and check the result, if there has some exceptions, "Sqoop2 server started." won't be displayed. > On Sept. 28, 2015, 4:29 p.m., Jarek Cecho wrote: > > dist/src/main/bin/sqoop.sh, line 27 > > <https://reviews.apache.org/r/38589/diff/2/?file=1084669#file1084669line27> > > > > In the old Tomcat world [1] we had default of SQOOP_HOME/lib (masked as > > ${catalina.home}/../lib/*.jar) that allowed user to create lib/ directory, > > put random jars (mostly JDBC drivers) and inject them easily to Sqoop 2 > > server classpath. I'm wondering whether we can provide similar > > functionality? Let's perhaps define variable SQOOP_SERVER_EXTRA_LIB that > > defaults to "lib/" that enables admin to easily inject the jars? > > > > I can see the variable SQOOP_SERVER_LIB pointing to our own directory, > > but I would prefer user not to mess with our own directories and have a > > dedicated directory for "inserted" jars. It's better for upgrade as you can > > simply point the new version to the same "extra" directory without need to > > see what jars belongs to Sqoop and what were added and re-add them to the > > new version. > > > > Links: > > 1: > > https://github.com/apache/sqoop/blob/sqoop2/dist/src/main/server/conf/catalina.properties Good suggestion, add the SQOOP_SERVER_EXTRA_LIB to the script. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38589/#review100815 ----------------------------------------------------------- On Sept. 29, 2015, 8:18 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38589/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2015, 8:18 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > Update script for Jetty server > > > Diffs > ----- > > dist/src/main/bin/sqoop-sys.sh 64dd0bf > dist/src/main/bin/sqoop.sh 707c3fc > > Diff: https://reviews.apache.org/r/38589/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
