> On Dec. 5, 2014, 8:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 465 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line465> > > > > a switch statement might be cleaner here > > Robert Levas wrote: > Changed to switch statement. It's not clear why this is cleaner... less > parens, maybe?
Generally, switch statements are preferred over if/else because they can be better optimized by the compiler and IMO are easier to read. > On Dec. 5, 2014, 8:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 310 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line310> > > > > less than ideal that you need to downcast here. To me this indicates > > an issue with your object model. > > Robert Levas wrote: > Not an issue with my object model... maybe my naming convention. :) > On Dec. 5, 2014, 8:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 411 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line411> > > > > IMO, it would be cleaner to also have a copy constructor that took > > another desctiptor instance. This would remove the need to convert to/from > > a map. > > Robert Levas wrote: > The current implement reduces the amount of code needed to create a deep > copy of the structure, but I will add it if pushed to do so. Depending on the performance improvment it could be worth it. Not a big deal, just wanted to see if you considered it. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28705/#review63945 ----------------------------------------------------------- On Dec. 8, 2014, 5:45 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28705/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2014, 5:45 a.m.) > > > Review request for Ambari, dilli dorai, Jaimin Jetly, Jonathan Hurley, John > Speidel, and Robert Nettleton. > > > Bugs: AMBARI-8542 > https://issues.apache.org/jira/browse/AMBARI-8542 > > > Repository: ambari > > > Description > ------- > > Provide the ability to read in Kerberos descriptor files (kerberos.json) from > the stack at various levels (stack-level, service-level) and to merge them > into a single hierarchy. The composite Kerberos descriptor data will be used > to control the UI (Kerberos Wizard - see AMBARI-7450). > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosComponentDescriptor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosConfigurationDescriptor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptorType.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosIdentityDescriptor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosKeytabDescriptor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosPrincipalDescriptor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptor.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosComponentDescriptorTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosConfigurationDescriptorTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosDescriptorTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosIdentityDescriptorTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosKeytabDescriptorTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosPrincipalDescriptorTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptorTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/28705/diff/ > > > Testing > ------- > > Added new unit tests: > Running org.apache.ambari.server.state.kerberos.KerberosServiceDescriptorTest > Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.2 sec > Running > org.apache.ambari.server.state.kerberos.KerberosComponentDescriptorTest > Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.189 sec > Running > org.apache.ambari.server.state.kerberos.KerberosConfigurationDescriptorTest > Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.206 sec > Running org.apache.ambari.server.state.kerberos.KerberosDescriptorTest > Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.268 sec > Running org.apache.ambari.server.state.kerberos.KerberosIdentityDescriptorTest > Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.184 sec > Running org.apache.ambari.server.state.kerberos.KerberosKeytabDescriptorTest > Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.179 sec > Running > org.apache.ambari.server.state.kerberos.KerberosPrincipalDescriptorTest > > > Tests run: 2364, Failures: 0, Errors: 0, Skipped: 22 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 37:38 min > [INFO] Finished at: 2014-12-04T16:36:31+00:00 > [INFO] Final Memory: 42M/486M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Robert Levas > >
