Return-Path: X-Original-To: apmail-ambari-dev-archive@www.apache.org Delivered-To: apmail-ambari-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 247CE10A7E for ; Fri, 12 Dec 2014 21:37:31 +0000 (UTC) Received: (qmail 83852 invoked by uid 500); 12 Dec 2014 21:37:31 -0000 Delivered-To: apmail-ambari-dev-archive@ambari.apache.org Received: (qmail 83820 invoked by uid 500); 12 Dec 2014 21:37:31 -0000 Mailing-List: contact dev-help@ambari.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@ambari.apache.org Delivered-To: mailing list dev@ambari.apache.org Received: (qmail 83805 invoked by uid 99); 12 Dec 2014 21:37:30 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 12 Dec 2014 21:37:30 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 33D5B1D239E; Fri, 12 Dec 2014 21:37:27 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============4613623393107604797==" MIME-Version: 1.0 Subject: Re: Review Request 28944: Add Kerberos Configuration Metadata File Builder and Reader From: "John Speidel" To: "John Speidel" , "dilli dorai" , "Robert Nettleton" Cc: "Robert Levas" , "Ambari" Date: Fri, 12 Dec 2014 21:37:27 -0000 Message-ID: <20141212213727.5765.99939@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "John Speidel" X-ReviewGroup: Ambari X-ReviewRequest-URL: https://reviews.apache.org/r/28944/ X-Sender: "John Speidel" References: <20141211160444.26419.42230@reviews.apache.org> In-Reply-To: <20141211160444.26419.42230@reviews.apache.org> Reply-To: "John Speidel" X-ReviewRequest-Repository: ambari --===============4613623393107604797== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 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 just checking that this class doesn' need to be thread safe ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileBuilder.java could inline writeHeader as it is only used here ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java implement -> implements ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java why 2 empty lines between methods? ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java 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 NPE if already closed ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java an Iterator of what? ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/AbstractKerberosDataFileReader.java NPE if closed ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFile.java 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 javadocs for constants ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFile.java why an empty line here? ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileBuilder.java See comment in KerberosDataFile about using static imports instead of inheritence. ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosConfigDataFileReader.java 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 > > --===============4613623393107604797==--