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 8B41610B5D for ; Fri, 5 Dec 2014 20:08:33 +0000 (UTC) Received: (qmail 21175 invoked by uid 500); 5 Dec 2014 20:08:33 -0000 Delivered-To: apmail-ambari-dev-archive@ambari.apache.org Received: (qmail 21145 invoked by uid 500); 5 Dec 2014 20:08:33 -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 20475 invoked by uid 99); 5 Dec 2014 20:08:32 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 05 Dec 2014 20:08:32 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 3908D1D2271; Fri, 5 Dec 2014 20:08:30 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============2895829761916493718==" MIME-Version: 1.0 Subject: Re: Review Request 28705: Provide a way to parse and handle Kerberos descriptors From: "John Speidel" To: "John Speidel" , "dilli dorai" , "Robert Nettleton" , "Jaimin Jetly" , "Jonathan Hurley" Cc: "Robert Levas" , "Ambari" Date: Fri, 05 Dec 2014 20:08:30 -0000 Message-ID: <20141205200830.32525.46322@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/28705/ X-Sender: "John Speidel" References: <20141204175748.32525.82407@reviews.apache.org> In-Reply-To: <20141204175748.32525.82407@reviews.apache.org> Reply-To: "John Speidel" X-ReviewRequest-Repository: ambari --===============2895829761916493718== 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/28705/#review63945 ----------------------------------------------------------- ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java "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 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 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 Why is this check necessary? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptor.java 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 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 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 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 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 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 would be nice to have a constant for "identities" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java would be nice to have a constant for "configurations" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java 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 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 if -> If ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java 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 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 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 a switch statement might be cleaner here ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/AbstractKerberosDescriptorContainer.java 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 would be nice to have a constant for "identity" and "configuration" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosConfigurationDescriptor.java putAll() vs. iterate/put() ? ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java fyi, not currently used ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java fyi, not currently used ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java would be nice to have constants for "services" and "properties" ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosDescriptor.java message for exception ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosKeytabDescriptor.java 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 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 fyi, this method is never used ambari-server/src/main/java/org/apache/ambari/server/state/kerberos/KerberosServiceDescriptor.java 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 This method isn't used, was this an oversight? ambari-server/src/test/java/org/apache/ambari/server/state/kerberos/KerberosIdentityDescriptorTest.java 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 > > --===============2895829761916493718==--