ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 32589: Principal and Keytab configuration specifications are ignored when disabling Kerberos
Date Mon, 30 Mar 2015 15:23:40 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32589/#review78207
-----------------------------------------------------------

Ship it!


Looks fine, mostly just some general comments that you are free to ignore.


ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java
<https://reviews.apache.org/r/32589/#comment126715>

    Inconsistent, in some cases you use a static import and in other don't



ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java
<https://reviews.apache.org/r/32589/#comment126714>

    seems that this should be moved out of the for block as it is specific to type and not
the property.



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFile.java
<https://reviews.apache.org/r/32589/#comment126717>

    strange to have a class with no behavior



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileBuilder.java
<https://reviews.apache.org/r/32589/#comment126716>

    I know that is was just renamed from KerberosActionDataFileBuilder but couldn't resist
commenting on the name (was also the same prior to the rename).  Perhaps it is because it
is Monday morning ;)
    
    Calling a class *Builder implies that a class is an implementation of the well known Builder
Pattern but none of he Kerberos*Builder classes are.
    
    No need to change anything, just commenting.



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileFactory.java
<https://reviews.apache.org/r/32589/#comment126719>

    Strange that a KerberosIdentityDataFile factory returns a builder


- John Speidel


On March 29, 2015, 12:24 p.m., Robert Levas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32589/
> -----------------------------------------------------------
> 
> (Updated March 29, 2015, 12:24 p.m.)
> 
> 
> Review request for Ambari, John Speidel and Robert Nettleton.
> 
> 
> Bugs: AMBARI-10236
>     https://issues.apache.org/jira/browse/AMBARI-10236
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Principal and Keytab configuration specifications are ignored when disabling Kerberos
and thus they are not updated or removed as needed.
> 
> **Solution**
> Move the configuration details for principal and keytab items from the action identities
data file to the configurations data file. The existing logic will handle the rest.
> 
> Note: Cleanup of some code was done. For example Kerberos*Action*Data* classes were renamed
to Kerberos*Identity*Data*. Also, factory classes were added to create the Kerberos*DataFile(Builder/Reader)
classes.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java 9f39049

>   ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java
75062a1 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
2d9f98a 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreateKeytabFilesServerAction.java
3e94cd6 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/CreatePrincipalsServerAction.java
e8918e1 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFile.java
e85048d 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileBuilder.java
31e62be 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileReader.java
cf872ca 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileBuilder.java
a10f38e 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileFactory.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileReader.java
a230814 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFile.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileBuilder.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileFactory.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileReader.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java
73a4ad6 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/UpdateKerberosConfigsServerAction.java
9e342d0 
>   ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatHandler.java
5541523 
>   ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java
e451ad1 
>   ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileTest.java
b467760 
>   ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileTest.java
413de0b 
>   ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileTest.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerActionTest.java
b0345aa 
>   ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/UpdateKerberosConfigsServerActionTest.java
23ab519 
> 
> Diff: https://reviews.apache.org/r/32589/diff/
> 
> 
> Testing
> -------
> 
> Manually tested to see that relevant principal and keytab configuratations were removed
as necessary.
> 
> **Local test results**
> Running org.apache.ambari.server.agent.TestHeartbeatHandler
> Tests run: 34, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 69.902 sec
> 
> Running org.apache.ambari.server.controller.KerberosHelperTest
> Tests run: 25, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.135 sec
> 
> Running org.apache.ambari.server.serveraction.kerberos.KerberosConfigDataFileTest
> Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.1 sec
> 
> Running org.apache.ambari.server.serveraction.kerberos.KerberosIdentityDataFileTest
> Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.107 sec
> 
> Running org.apache.ambari.server.serveraction.kerberos.UpdateKerberosConfigsServerActionTest
> Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.596 sec
> 
> Tests run: 2802, Failures: 0, Errors: 0, Skipped: 15
> 
> **Jenkins test results:**
> *Failed due to `Failed to execute goal com.github.eirslett:frontend-maven-plugin:0.0.14:npm
(npm install) on project ambari-admin: 'npm install --unsafe-perm --color=false' failed. (error
code 1) -> [Help 1]`*
> 
> 
> Thanks,
> 
> Robert Levas
> 
>


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