----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28705/#review63945 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106292> "It provides storage and management the parent" Seems that words are missing? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106308> Why are the outermost '(' ')' needed. This capture group encompasses the entire expression which is always available via group(0) if you need it. If you remove the outer capture group, then you will need to adjust the group numbers in replaceVariables. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106380> Why check find() here, just so that you don't need to create the sb if there is no match? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106378> Why is this check necessary? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106377> Is this an exception condition? At a minimum we should log that we weren't able to fully resolve the variable. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106384> Should state the conditions in which null is returned. Would it be better to return Collections.emptyMap() instead of null? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106383> 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? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106386> Should specify the conditions in which null will be returned. Would it be better to return Collections.emptyMap() instead of null? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106387> if the above methods returned an empty map instead of null it seems that this method wouldn't be needed. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106388> Is there ever a case where an implementing class wouldn't override this method? If not, then this method should be abstract. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106389> would be nice to have a constant for "identities" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106390> would be nice to have a constant for "configurations" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106392> Should state the conditions in which null is returned. Would it be better to return Collections.emptyList() instead of null? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106393> I assume that this class doesn't need to be thread safe ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106394> if -> If ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106395> seems wonky that you need to use an instanceof check to determine if the descriptor is a container. Perhaps add a isContainer() method to the interface instead? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106396> less than ideal that you need to downcast here. To me this indicates an issue with your object model. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106398> 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. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106429> a switch statement might be cleaner here ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106430> you should probably handle the case where you have an unexpected pathParts length. Perhaps an exception should be generated? At a minimum log an error msg. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java <https://reviews.apache.org/r/28705/#comment106432> would be nice to have a constant for "identity" and "configuration" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosConfigurationDescriptor.java <https://reviews.apache.org/r/28705/#comment106439> putAll() vs. iterate/put() ? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106442> fyi, not currently used ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106441> fyi, not currently used ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106445> would be nice to have constants for "services" and "properties" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java <https://reviews.apache.org/r/28705/#comment106450> message for exception ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosKeytabDescriptor.java <https://reviews.apache.org/r/28705/#comment106458> Since this doesn't return a File instance, perhaps getFilePath() would be clearer. ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosPrincipalDescriptor.java <https://reviews.apache.org/r/28705/#comment106468> for both set* calls in this method you should just set updatedValue instead of invoking the get* method again on updates ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptor.java <https://reviews.apache.org/r/28705/#comment106469> fyi, this method is never used ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptor.java <https://reviews.apache.org/r/28705/#comment106473> Why would a component with a null name be passed into putComponent? Seems odd that it is just ignored in this case. Should a non-null name be an invariant that results in an exception if the invariant is violated. I think that I have seen this pattern in other places in the patch where if some value is null, the operation is basically a no-op. Seems dangerous to me. ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosComponentDescriptorTest.java <https://reviews.apache.org/r/28705/#comment106476> This method isn't used, was this an oversight? ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosIdentityDescriptorTest.java <https://reviews.apache.org/r/28705/#comment106478> unused import - John Speidel On Dec. 4, 2014, 5: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, 5: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 > >
