----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52399/#review151159 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/util/Instrumentation.java (line 783) <https://reviews.apache.org/r/52399/#comment219333> XLog.format() creates only a String that is not used. core/src/main/java/org/apache/oozie/util/Instrumentation.java (line 784) <https://reviews.apache.org/r/52399/#comment219332> Dead code, pls. remove. distro/src/main/bin/oozie-jetty-server.sh (line 21) <https://reviews.apache.org/r/52399/#comment219348> setup_jetty_log_and_pid() distro/src/main/bin/oozie-jetty-server.sh (lines 29 - 34) <https://reviews.apache.org/r/52399/#comment219334> Can be extracted to a method: check_and_set_env_var() distro/src/main/bin/oozie-jetty-server.sh (lines 69 - 71) <https://reviews.apache.org/r/52399/#comment219338> I'd rather extract the conditions to one variable called isPidPresent, making the three ifs way more readable. distro/src/main/bin/oozie-jetty-server.sh (line 146) <https://reviews.apache.org/r/52399/#comment219343> RETRY_COUNT distro/src/main/bin/oozie-jetty-server.sh (line 150) <https://reviews.apache.org/r/52399/#comment219344> ${RETRY_COUNT} distro/src/main/bin/oozie-jetty-server.sh (line 167) <https://reviews.apache.org/r/52399/#comment219347> I think neither the variable FORCE nor this condition is not needed since ${FORCE} always evaluates to 0. distro/src/main/bin/oozie-jetty-server.sh (lines 181 - 184) <https://reviews.apache.org/r/52399/#comment219349> Extract method check_and_create_webapp() instead of comment. pom.xml (line 105) <https://reviews.apache.org/r/52399/#comment219331> What about jetty.version instead? server/pom.xml (line 26) <https://reviews.apache.org/r/52399/#comment219363> Either I'd put also the jetty.version property here instead of the parent pom.xml (if the jetty dependencies are used only from oozie-server artifact), or I'd put the dependencies' versions to parent pom.xml and omit those here (if the jetty dependencies are used not only from oozie-server artifact). server/pom.xml (lines 350 - 376) <https://reviews.apache.org/r/52399/#comment219366> Formatting issue. server/pom.xml (line 375) <https://reviews.apache.org/r/52399/#comment219364> Is that a version string we should also extract? server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (line 44) <https://reviews.apache.org/r/52399/#comment219367> Why not private? server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (lines 79 - 80) <https://reviews.apache.org/r/52399/#comment219368> You can go ahead w/ Integer.getInteger(String, int). server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (lines 79 - 101) <https://reviews.apache.org/r/52399/#comment219370> Why not use a Builder pattern here as in: https://github.com/iluwatar/java-design-patterns/tree/master/builder server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (lines 87 - 88) <https://reviews.apache.org/r/52399/#comment219369> You can go ahead w/ Integer.getInteger(String, int). server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (lines 105 - 111) <https://reviews.apache.org/r/52399/#comment219376> Does it not belong to a nested class / to another class instead? server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (line 107) <https://reviews.apache.org/r/52399/#comment219371> Extract constant value. server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (line 108) <https://reviews.apache.org/r/52399/#comment219372> Extract constant value. server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (lines 114 - 115) <https://reviews.apache.org/r/52399/#comment219373> Integer.getInteger(String) server/src/main/java/org/apache/oozie/server/EmbeddedOozieServer.java (lines 128 - 134) <https://reviews.apache.org/r/52399/#comment219374> Should it be part of a unit test instead? Who will call it? server/src/main/java/org/apache/oozie/server/JspHandler.java (lines 51 - 60) <https://reviews.apache.org/r/52399/#comment219379> What about this one instead: https://docs.oracle.com/javase/7/docs/api/java/io/File.html#createTempFile(java.lang.String,%20java.lang.String,%20java.io.File) server/src/main/java/org/apache/oozie/server/JspHandler.java (line 85) <https://reviews.apache.org/r/52399/#comment219381> Extract to constant field w/ Javadoc. server/src/main/java/org/apache/oozie/server/JspHandler.java (line 106) <https://reviews.apache.org/r/52399/#comment219399> Lists.newArrayList() or new ArrayList<>() server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (lines 42 - 44) <https://reviews.apache.org/r/52399/#comment219400> // @formatter:off private static String[] A = { B, C, D, E}; // @formatter:on server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (line 48) <https://reviews.apache.org/r/52399/#comment219401> environmentVariables? server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (line 63) <https://reviews.apache.org/r/52399/#comment219402> Do we really need it here? If so, why not call the method setEnvAndCreateSecureServerConnector()? server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (lines 64 - 70) <https://reviews.apache.org/r/52399/#comment219407> Instead of it maybe try Builder pattern. server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (lines 102 - 103) <https://reviews.apache.org/r/52399/#comment219403> Extract method server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (lines 108 - 109) <https://reviews.apache.org/r/52399/#comment219404> Extract method server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (lines 114 - 115) <https://reviews.apache.org/r/52399/#comment219405> Extract method server/src/main/java/org/apache/oozie/server/SSLServerConnectorFactory.java (lines 120 - 121) <https://reviews.apache.org/r/52399/#comment219406> Extract method server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java (lines 31 - 36) <https://reviews.apache.org/r/52399/#comment219408> All can be private. server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java (line 44) <https://reviews.apache.org/r/52399/#comment219412> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java (lines 47 - 48) <https://reviews.apache.org/r/52399/#comment219409> verifyNoMoreInteractions(mock*); server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java (line 52) <https://reviews.apache.org/r/52399/#comment219411> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestEmbeddedOozieServer.java (lines 65 - 67) <https://reviews.apache.org/r/52399/#comment219410> verifyNoMoreInteractions(mock*); server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 29 - 34) <https://reviews.apache.org/r/52399/#comment219413> All can be private. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 42) <https://reviews.apache.org/r/52399/#comment219414> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 45 - 48) <https://reviews.apache.org/r/52399/#comment219420> Extract method. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 61) <https://reviews.apache.org/r/52399/#comment219415> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 64 - 67) <https://reviews.apache.org/r/52399/#comment219421> Extract method. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 81) <https://reviews.apache.org/r/52399/#comment219416> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 84 - 87) <https://reviews.apache.org/r/52399/#comment219422> Extract method. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 101) <https://reviews.apache.org/r/52399/#comment219417> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 104 - 107) <https://reviews.apache.org/r/52399/#comment219423> Extract method. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 113 - 115) <https://reviews.apache.org/r/52399/#comment219426> More elegant solution would be to use an enum. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 119) <https://reviews.apache.org/r/52399/#comment219418> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 122 - 125) <https://reviews.apache.org/r/52399/#comment219424> Extract method. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 127 - 129) <https://reviews.apache.org/r/52399/#comment219427> More elegant solution would be to use an enum. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (line 140) <https://reviews.apache.org/r/52399/#comment219419> By only reading the test method names, we should be able to conclude: 1) assumptions 2) when something happens... 3) ...then some other thing should happen server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 143 - 146) <https://reviews.apache.org/r/52399/#comment219425> Extract method. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 148 - 150) <https://reviews.apache.org/r/52399/#comment219428> More elegant solution would be to use an enum. server/src/test/java/org/apache/oozie/server/TestSSLServerConnectorFactory.java (lines 156 - 158) <https://reviews.apache.org/r/52399/#comment219429> More elegant solution would be to use an enum. - András Piros On Sept. 30, 2016, 3:34 p.m., Attila Sasvari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52399/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2016, 3:34 p.m.) > > > 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 > >
