> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5292-5294
> > <https://reviews.apache.org/r/51822/diff/1/?file=1497122#file1497122line5292>
> >
> >     Why not just return here which would prevent you from needing to embed 
> > the rest of the logic in the if-statement
> 
> Robert Levas wrote:
>     Then I would have 2 return statements... but I can make the chance since 
> most developers like to short-curcuit their code.

That's true; it's 50/50 on what preferences are for this stuff. I find that 
with Ambari, we have a lot of nested if-statements, so we end up with code like:
```
if(null != cluster)
  if(null != cluster.getServices())
    if(null != service.getName())
      // do something
      
```
Where the something is quite complex and has it's own control flow. Because of 
this, I think it's easier and less error prone to quickly return for nulls.


> On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java,
> >  lines 5298-5301
> > <https://reviews.apache.org/r/51822/diff/1/?file=1497122#file1497122line5298>
> >
> >     KERBEROS_AUTH_USER_TYPES has a default value, so `getProperty` will 
> > never return null.
> 
> Robert Levas wrote:
>     This is a safety measure in the event someone changes the definition of 
> `KERBEROS_AUTH_USER_TYPES`. Plus I generally feel the need to check for null 
> and empty in these cases. But I can remove this check.

Nope, that's fine - you can keep it then - I was just pointing out that you're 
getting the default for free here so unless somebody sets it to blank in 
`ambari.properties` it should never be null.


- Jonathan


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


On Sept. 13, 2016, 4:44 p.m., Robert Levas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51822/
> -----------------------------------------------------------
> 
> (Updated Sept. 13, 2016, 4:44 p.m.)
> 
> 
> Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, and Nate 
> Cole.
> 
> 
> Bugs: AMBARI-18365
>     https://issues.apache.org/jira/browse/AMBARI-18365
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Add the followng Ambari configuration options to support Kerberos token 
> authentication
> 
> - `authentication.kerberos.enabled`
> -- Determines whether to use Kerberos (SPNEGO) authentication when connecting 
> Ambari:  {{true}} to enable this feature; {{false}}, otherwise
> - `authentication.kerberos.spnego.principal`
> -- The Kerberos principal name to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.spnego.keytab.file`
> -- The Kerberos keytab file to use when verifying user-supplied Kerberos 
> tokens for authentication via SPNEGO
> - `authentication.kerberos.user.types`
> -- A comma-delimited (ordered) list of preferred user types to use when 
> finding the Ambari user account for the user-supplied Kerberos identity 
> during authentication via SPNEGO
> - `authentication.kerberos.auth_to_local.rules`
> -- The auth-to-local rules set to use when translating a user's principal 
> name to a local user name during authentication via SPNEGO.
> 
> NOTE: These properties are in the {{ambari.properties}} file since this 
> feature may be enabled whether the rest of the cluster has Kerberos enabled 
> or not. 
> 
> See https://issues.apache.org/jira/browse/AMBARI-18364 for an overview 
> requiring the need for this patch.
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java
>  ee73b8d 
>   
> ambari-server/src/main/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationProperties.java
>  PRE-CREATION 
>   
> ambari-server/src/test/java/org/apache/ambari/server/configuration/ConfigurationTest.java
>  f429a36 
>   
> ambari-server/src/test/java/org/apache/ambari/server/security/authentication/kerberos/AmbariKerberosAuthenticationPropertiesTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51822/diff/
> 
> 
> Testing
> -------
> 
> Manually tested...
> 
> # Local test results: 
> 
> [INFO] 
> ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] 
> ------------------------------------------------------------------------
> [INFO] Total time: 2:51:23.081s
> [INFO] Finished at: Tue Sep 13 00:00:59 EDT 2016
> [INFO] Final Memory: 48M/1792M
> [INFO] 
> ------------------------------------------------------------------------
> 
> # Jenkins test results: 
> 
> [INFO] 
> ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] 
> ------------------------------------------------------------------------
> [INFO] Total time: 01:34 h
> [INFO] Finished at: 2016-09-13T08:46:23+00:00
> [INFO] Final Memory: 178M/2672M
> [INFO] 
> ------------------------------------------------------------------------
> 
> {color:green}+1 overall{color}.  Here are the results of testing the latest 
> attachment 
>   
> http://issues.apache.org/jira/secure/attachment/12828101/AMBARI-18365_trunk_01.patch
>   against trunk revision .
> 
>     {color:green}+1 @author{color}.  The patch does not contain any @author 
> tags.
> 
>     {color:green}+1 tests included{color}.  The patch appears to include 2 
> new or modified test files.
> 
>     {color:green}+1 javac{color}.  The applied patch does not increase the 
> total number of javac compiler warnings.
> 
>     {color:green}+1 release audit{color}.  The applied patch does not 
> increase the total number of release audit warnings.
> 
>     {color:green}+1 core tests{color}.  The patch passed unit tests in 
> ambari-server.
> 
> Test results: 
> https://builds.apache.org/job/Ambari-trunk-test-patch/8643//testReport/
> Console output: 
> https://builds.apache.org/job/Ambari-trunk-test-patch/8643//console
> 
> This message is automatically generated.
> 
> 
> Thanks,
> 
> Robert Levas
> 
>

Reply via email to