----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52399/#review151176 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/util/Instrumentation.java (line 787) <https://reviews.apache.org/r/52399/#comment219433> Check white spaces here. distro/src/main/bin/oozie-jetty-server.sh (line 109) <https://reviews.apache.org/r/52399/#comment219435> I think we should also print an error if Jetty cannot not be started for whatever reason distro/src/main/bin/oozie-jetty-server.sh (line 167) <https://reviews.apache.org/r/52399/#comment219438> Can't we just use a single if? distro/src/main/bin/oozied.sh (line 101) <https://reviews.apache.org/r/52399/#comment219457> Inconsistent use of "test" distro/src/main/bin/oozied.sh (line 110) <https://reviews.apache.org/r/52399/#comment219439> Check for white spaces distro/src/main/bin/oozied.sh (line 115) <https://reviews.apache.org/r/52399/#comment219440> Is this TODO still relevant? server/pom.xml (line 26) <https://reviews.apache.org/r/52399/#comment219441> +1, I'd remove versions tags here server/src/main/assemblies/empty.xml (line 9) <https://reviews.apache.org/r/52399/#comment219442> Booo, trailing spaces :) server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (line 63) <https://reviews.apache.org/r/52399/#comment219444> Minor: I'd be paranoid and null-check all incoming dependencies here. server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (lines 79 - 80) <https://reviews.apache.org/r/52399/#comment219443> Extract strings to constants like "oozie.http.port", "OOZIE_HTTP_PORT", etc server/src/main/java/org/apache/oozie/server/JspHandler.java (lines 51 - 60) <https://reviews.apache.org/r/52399/#comment219445> extract properties to constant server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (line 46) <https://reviews.apache.org/r/52399/#comment219447> Minor: don't we have an enum somehwere for these protocols? server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (line 49) <https://reviews.apache.org/r/52399/#comment219448> Minor: add line-feed before constructor & null-check the factory server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (line 62) <https://reviews.apache.org/r/52399/#comment219450> Why capitals for OOZIE_HTTPS_PORT ? server/src/main/resources/checkstyle.xml (line 35) <https://reviews.apache.org/r/52399/#comment219456> Mind the formatting server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java (line 54) <https://reviews.apache.org/r/52399/#comment219451> extract property server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 38) <https://reviews.apache.org/r/52399/#comment219454> If this runs as a JUnit4 test then you can use a MockitoJunitRunner. See: http://site.mockito.org/mockito/docs/current/org/mockito/runners/MockitoJUnitRunner.html server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 51) <https://reviews.apache.org/r/52399/#comment219455> Extract "42" server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 73) <https://reviews.apache.org/r/52399/#comment219453> This list should be also accessible from a static variable. Or at least extract it to a constant in the test class. - Peter Bacsko On szept. 30, 2016, 3:34 du, Attila Sasvari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52399/ > ----------------------------------------------------------- > > (Updated szept. 30, 2016, 3:34 du) > > > Review request for oozie, András Piros, Peter Cseh, Peter Bacsko, and Robert > Kanter. > > > Repository: oozie-git > > > Description > ------- > > Embedding jetty into Oozie so that it can run as a standalone application. > The changes also try to address OOZIE-2317 (i.e. Tomcat 6 is EOL). > > New functionality > - New module (server) is added that sets up an embedded Jetty server and > start Oozie services. Servlet mapping is done by reading web.xml of webapp at > runtime. JSP is handled with custom code. Server version is not revealed in > server repsonses. > - SSL protocols and ciphers can be configured via system properties and > environment variables. Precedence: system properties, environment variables, > default values > > Changes in existing code > - Excluded jetty 6 dependencies from core and updated tests accordingly > > Packaging > - oozie.sh is modified so that it starts Oozie with embedded jetty by > default. If someone would like to use tomcat for any reason, they can set an > environment variable (e.g. OOZIE_USE_TOMCAT=1). > > TODO: > - Add more tests > - Add more documentation > - Code cleanup + refactoring in packaging and core parts > - Maven clean up > - Allow to tune more Jetty settings (for example threadpool) > - More security measures (e.g. protect against clickjacking, CSRF, etc.) > - Update Oozie Documentation > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/util/Instrumentation.java > fa1e92a0f1fadfb66c5e66fea5f26f57579080e7 > distro/src/main/bin/oozie-jetty-server.sh PRE-CREATION > distro/src/main/bin/oozied.sh a869c3da177c863a068f2af45c7bca9d5cb771ac > pom.xml 704a2eeee9f4e4805e3e08c2a547b2a375f6b1b2 > server/pom.xml PRE-CREATION > server/src/main/assemblies/empty.xml PRE-CREATION > server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java > PRE-CREATION > server/src/main/java/org/apache/oozie/server/JspHandler.java PRE-CREATION > server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java > PRE-CREATION > server/src/main/resources/checkstyle-header.txt PRE-CREATION > server/src/main/resources/checkstyle.xml PRE-CREATION > server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java > PRE-CREATION > > server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java > PRE-CREATION > src/main/assemblies/distro.xml 1ffbfd6d2ba33b390999e9094cbb336fbce45c21 > > Diff: https://reviews.apache.org/r/52399/diff/ > > > Testing > ------- > > - Tested basic functionality by executing a workflow that uses the sample > JavaAction > - without SSL - on a 2.4.0 pseudo Hadoop cluster > - SSL with Kerberos is using a test CDH cluster > - Added new unit tests that check > - If oozie.ssl.enabled is not specified, server starts without SSL > settings > - If oozie.ssl.enabled is specified, server starts with SSL settings > - SSL protocols and ciphers can be configured via system properties and > environment variables > - Ran subset of tests using Hadoop-2 profile > - mvn clean package assembly:single -DjavaVersion=1.8 > -DtargetVersion=1.7 -Dtest=TestJavaActionExecutor -Phadoop-2 > -Dhadoop.version=2.4.0 > > > Thanks, > > Attila Sasvari > >
