ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 28705: Provide a way to parse and handle Kerberos descriptors
Date Fri, 05 Dec 2014 20:08:30 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message