> On March 30, 2015, 11:23 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/agent/HeartBeatHandler.java, > > line 1066 > > <https://reviews.apache.org/r/32589/diff/1/?file=908264#file908264line1066> > > > > Inconsistent, in some cases you use a static import and in other don't
I noticed this. I think it IntelliJ has started to automatically create static imports - I don't do it since am not a fan of static imports. I am happy to make consistent. What does the community prefer? > On March 30, 2015, 11:23 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java, > > line 2503 > > <https://reviews.apache.org/r/32589/diff/1/?file=908265#file908265line2503> > > > > seems that this should be moved out of the for block as it is specific > > to type and not the property. Agreed... moving out. Thanks. > On March 30, 2015, 11:23 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFile.java, > > line 25 > > <https://reviews.apache.org/r/32589/diff/1/?file=908275#file908275line25> > > > > strange to have a class with no behavior Yeah... Someone suggested a change in some patch that lead to this and over time functionaly was moved out of it. Essentually if we had multiple inheritance, KerberosIdentityDataFileReader would extend AbstractKerberosDataFileReader and KerberosIdentityDataFile where KerberosIdentityDataFileBuilder would extend both AbstractKerberosDataFileBuilder and KerberosIdentityDataFile. Looking at the history, I think KerberosIdentityDataFile (renamed from KerberosActionDataFile) was an interface. I guess this made some reviewer upset since it didn't expose any methods. > On March 30, 2015, 11:23 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileBuilder.java, > > line 33 > > <https://reviews.apache.org/r/32589/diff/1/?file=908276#file908276line33> > > > > 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. I guess it should have been called KerberosIdentityDataFileWriter... bummer. Maybe if you push hard enough I will change this and the related constant containor classes. > On March 30, 2015, 11:23 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosIdentityDataFileFactory.java, > > line 42 > > <https://reviews.apache.org/r/32589/diff/1/?file=908277#file908277line42> > > > > Strange that a KerberosIdentityDataFile factory returns a builder Naming is a pain in the butt.. Do you have a better idea for the name of this? Maybe split it out to KerberosIdentityuFileWriterFactory and KerberosIdentityFileReaderFactory? - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32589/#review78207 ----------------------------------------------------------- On March 29, 2015, 8:24 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32589/ > ----------------------------------------------------------- > > (Updated March 29, 2015, 8:24 a.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 > >
