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