ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Nettleton" <rnettle...@hortonworks.com>
Subject Re: Review Request 28491: Create server-side actions to create kerberos principals and keytabs
Date Mon, 01 Dec 2014 16:47:08 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message