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