> On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 92 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line92> > > > > Why check find() here, just so that you don't need to create the sb if > > there is no match?
That and I had some other code in there at one point too. I will simplify. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 96 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line96> > > > > Why is this check necessary? In the habit of checking everying for saftey... I will simplify. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 124 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line124> > > > > Is this an exception condition? At a minimum we should log that we > > weren't able to fully resolve the variable. Taking the exception route... > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 205 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line205> > > > > Should state the conditions in which null is returned. Would it be > > better to return Collections.emptyMap() instead of null? Fixing to return Collections.emptyMap() > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 209 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line209> > > > > If the file isn't readable, should this result in an exception? Might > > also be a good idea to check if the file is a directory? If file is null, return Collections.emptyMap() If file is not a file or not readable, throws IOException If file is not found, throws FileNotFoundException Else lests Gson handle it > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 222 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line222> > > > > Should specify the conditions in which null will be returned. Would it > > be better to return Collections.emptyMap() instead of null? Fixing to return Collections.emptyMap() > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 241 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line241> > > > > if the above methods returned an empty map instead of null it seems > > that this method wouldn't be needed. It is still possible in some cases where that map could be null, just not from the methods adjusted from the previous comments. If you think I would be better, I can remove the getValue method an inline the calls, checking for null if needed. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java, > > line 269 > > <https://reviews.apache.org/r/28705/diff/1/?file=782670#file782670line269> > > > > Is there ever a case where an implementing class wouldn't override this > > method? If not, then this method should be abstract. Only `org.apache.ambari.server.state.kerberos.AbstractKerberosDescriptorContainer` and `org.apache.ambari.server.state.kerberos.KerberosServiceDescriptor` override this method. All other classes implementing `org.apache.ambari.server.state.kerberos.AbstractKerberosDescriptor` used the default method. This includes `org.apache.ambari.server.state.kerberos.KerberosConfigurationDescriptor`, `org.apache.ambari.server.state.kerberos.KerberosIdentityDescriptor` and a few others. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 98 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line98> > > > > would be nice to have a constant for "identities" Created `org.apache.ambari.server.state.kerberos.KerberosDescriptorType` to help with this issue. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 108 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line108> > > > > would be nice to have a constant for "configurations" Created `org.apache.ambari.server.state.kerberos.KerberosDescriptorType` to help with this issue. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 155 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line155> > > > > Should state the conditions in which null is returned. Would it be > > better to return Collections.emptyList() instead of null? Now returning Collections.emptyList() as suggested. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 226 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line226> > > > > I assume that this class doesn't need to be thread safe This is only used in a single thread to parse and merge Kerberos descriptor data. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java, > > line 140 > > <https://reviews.apache.org/r/28705/diff/1/?file=782674#file782674line140> > > > > would be nice to have constants for "services" and "properties" Created `org.apache.ambari.server.state.kerberos.KerberosDescriptorType` to help with this issue. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java, > > line 520 > > <https://reviews.apache.org/r/28705/diff/1/?file=782671#file782671line520> > > > > would be nice to have a constant for "identity" and "configuration" Created `org.apache.ambari.server.state.kerberos.KerberosDescriptorType` to help with this issue. > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java, > > line 106 > > <https://reviews.apache.org/r/28705/diff/1/?file=782674#file782674line106> > > > > fyi, not currently used It will be when the Kerberos Automation implementation is complete > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java, > > line 123 > > <https://reviews.apache.org/r/28705/diff/1/?file=782674#file782674line123> > > > > fyi, not currently used It will be when the Kerberos Automation implementation is complete > On Dec. 5, 2014, 3:08 p.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptor.java, > > line 118 > > <https://reviews.apache.org/r/28705/diff/1/?file=782678#file782678line118> > > > > fyi, this method is never used It will be when the Kerberos Automation implementation is complete > On Dec. 5, 2014, 3: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. Not an issue with my object model... maybe my naming convention. > On Dec. 5, 2014, 3: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 Changed to switch statement. It's not clear why this is cleaner... less parens, maybe? - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28705/#review63945 ----------------------------------------------------------- On Dec. 4, 2014, 12:57 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28705/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2014, 12:57 p.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/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 > >
