----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28491/#review63371 -----------------------------------------------------------
Ship it! Looks good, just added some minor issues to consider. ambari-server/pom.xml <https://reviews.apache.org/r/28491/#comment105599> Since this dependency is also added to the ambari-project pom.xml, adding it here should not be necessary. The ambari-server pom.xml should just pull it in from it's parent pom.xml. I'd recommend removing it from here. ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosOperationHandler.java <https://reviews.apache.org/r/28491/#comment105611> Might want to consider throwing an exception back to the caller for the error conditions at the top of this method. If any of these parameters are null, then someone will have to analyze the log to try and determine why something failed. A stack trace with this message might be more straight-forward, unless there is a compelling reason not to throw an exception here. ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosOperationHandler.java <https://reviews.apache.org/r/28491/#comment105612> Same as above, the error-checking code on the parameters should probably throw an exception. ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java <https://reviews.apache.org/r/28491/#comment105613> Minor issue: Should this javadoc read: "a Map to be used as..." ? ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java <https://reviews.apache.org/r/28491/#comment105614> Same minor issue as above in the javadoc, probably should read "a Map to be used as..." ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/MITKerberosOperationHandler.java <https://reviews.apache.org/r/28491/#comment105615> I'd recommend throwing an IllegalArgumentException here, instead of logging, to make debugging this simpler in the future. - Robert Nettleton On Nov. 27, 2014, 12:19 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28491/ > ----------------------------------------------------------- > > (Updated Nov. 27, 2014, 12:19 a.m.) > > > Review request for Ambari, dilli dorai, Jaimin Jetly, Jonathan Hurley, Nate > Cole, Robert Nettleton, and Tom Beerbower. > > > Bugs: AMBARI-8454 > https://issues.apache.org/jira/browse/AMBARI-8454 > > > Repository: ambari > > > Description > ------- > > Create server-side actions to generate the Kerberos principals and keytabs. > These actions will be used when setting up Kerberos for services when > Kerberizing a cluster. > > > Diffs > ----- > > ambari-project/pom.xml edba1dc > ambari-server/pom.xml e03b626 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreateKeytabFilesServerAction.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreatePrincipalsServerAction.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KDCType.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFile.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileBuilder.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileReader.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosCredential.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosOperationHandler.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosOperationHandlerFactory.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/MITKerberosOperationHandler.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosOperationHandlerTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerActionTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/28491/diff/ > > > Testing > ------- > > Added new unit tests: > > Running > org.apache.ambari.server.serveraction.kerberos.KerberosOperationHandlerTest > Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.676 sec > Running > org.apache.ambari.server.serveraction.kerberos.KerberosActionDataFileTest > Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.12 sec > Running > org.apache.ambari.server.serveraction.kerberos.KerberosServerActionTest > Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.465 sec > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 25:36.295s > [INFO] Finished at: Wed Nov 26 18:05:30 EST 2014 > [INFO] Final Memory: 43M/739M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Robert Levas > >
