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 28944: Add Kerberos Configuration Metadata File Builder and Reader
Date Fri, 12 Dec 2014 21:37:27 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28944/#review64968
-----------------------------------------------------------



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileBuilder.java
<https://reviews.apache.org/r/28944/#comment107800>

    Just checking that this comment makes sense to you as it doesn't to me.



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileBuilder.java
<https://reviews.apache.org/r/28944/#comment107801>

    just checking that this class doesn' need to be thread safe



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileBuilder.java
<https://reviews.apache.org/r/28944/#comment107811>

    could inline writeHeader as it is only used here



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
<https://reviews.apache.org/r/28944/#comment107802>

    implement -> implements



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
<https://reviews.apache.org/r/28944/#comment107816>

    why 2 empty lines between methods?



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
<https://reviews.apache.org/r/28944/#comment107817>

    Seems that this class and  AbstractKerberosDataFileBuilder could be in the same class
hierarchy since they share many methods.
    
    I noticied that this implementatin of open() is a bit different that the implementation
in AbstractKerberosDataFileBuilder.  Should file be checked for null as it is in AbstractKerberosDataFileBuilder?



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
<https://reviews.apache.org/r/28944/#comment107818>

    NPE if already closed



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
<https://reviews.apache.org/r/28944/#comment107819>

    an Iterator of what?



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
<https://reviews.apache.org/r/28944/#comment107821>

    NPE if closed



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFile.java
<https://reviews.apache.org/r/28944/#comment107826>

    This is a design pattern that has fallen out of favor in the java community.  By placing
these constants in an interface and then inheriting them, they become part of the api when
they are really an implementation detail.  Additionally, the hierarchy of the implementing
class is now odd in that it is implementing this class with no symantic meaning.
    
    Would be better to use static imports for this.



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFile.java
<https://reviews.apache.org/r/28944/#comment107823>

    javadocs for constants



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFile.java
<https://reviews.apache.org/r/28944/#comment107822>

    why an empty line here?



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileBuilder.java
<https://reviews.apache.org/r/28944/#comment107827>

    See comment in KerberosDataFile about using static imports instead of inheritence.



ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileReader.java
<https://reviews.apache.org/r/28944/#comment107828>

    See comment in KerberosDataFile about using static imports instead of inheritence. 
    
    Also, it is odd to extend a class and provide no new or modified behavior.


- John Speidel


On Dec. 11, 2014, 4:04 p.m., Robert Levas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28944/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2014, 4:04 p.m.)
> 
> 
> Review request for Ambari, dilli dorai, John Speidel, and Robert Nettleton.
> 
> 
> Bugs: AMBARI-8657
>     https://issues.apache.org/jira/browse/AMBARI-8657
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Provide a facility to write (build) and read a (temporary) file used to hold data needed
for updating the configuration data of services to be configured for Kerberos.
> 
> The format of the file should be hidden from the user of this facility, but will be CSV.
> 
> Added new classes:
> * org.apache.ambari.server.serveraction.kerberos.AbstractKerberosDataFileBuilder (mostly
code moved from org.apache.ambari.server.serveraction.kerberos.KerberosActionDataFileBuilder)
> * org.apache.ambari.server.serveraction.kerberos.AbstractKerberosDataFileReader (mostly
code moved from org.apache.ambari.server.serveraction.kerberos.KerberosActionDataFileReader)
> * org.apache.ambari.server.serveraction.kerberos.KerberosConfigDataFile
> * org.apache.ambari.server.serveraction.kerberos.KerberosConfigDataFileBuilder
> * org.apache.ambari.server.serveraction.kerberos.KerberosConfigDataFileReader
> * org.apache.ambari.server.serveraction.kerberos.KerberosConfigDataFileTest
> 
> Updated existing classes to implement abstract builder and reader :
> * org.apache.ambari.server.serveraction.kerberos.KerberosActionDataFileBuilder
> * org.apache.ambari.server.serveraction.kerberos.KerberosActionDataFileReader
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileBuilder.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileBuilder.java
b19e6f4 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosActionDataFileReader.java
f3d93f5 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFile.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileBuilder.java
PRE-CREATION 
>   ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileReader.java
PRE-CREATION 
>   ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/28944/diff/
> 
> 
> Testing
> -------
> 
> Additional unit test: org.apache.ambari.server.serveraction.kerberos.KerberosConfigDataFileTest
> Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.087 sec
> 
> Existing unit test: org.apache.ambari.server.serveraction.kerberos.KerberosActionDataFileTest
> Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.102 sec
> 
> 
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 26:46.040s
> [INFO] Finished at: Thu Dec 11 10:39:48 EST 2014
> [INFO] Final Memory: 45M/774M
> [INFO] ------------------------------------------------------------------------
> 
> 
> Thanks,
> 
> Robert Levas
> 
>


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