> 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
Then I would have 2 return statements... but I can make the chance since most developers like to short-curcuit their code. > 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. 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. > On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java, > > lines 5310-5311 > > <https://reviews.apache.org/r/51822/diff/1/?file=1497122#file1497122line5310> > > > > Invalid types would seem to be a mis-configuration which could cause > > connectivity issues with Ambari; did you want to instead throw a real > > exception here? ok.. that is an option. > On Sept. 13, 2016, 8:24 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/configuration/Configuration.java, > > lines 5345-5348 > > <https://reviews.apache.org/r/51822/diff/1/?file=1497122#file1497122line5345> > > > > Same question as above - because these configurations involve > > connection to Ambari, should we instead throw exceptions and fail startup > > on problems like this? ok.. will fix. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51822/#review148699 ----------------------------------------------------------- On Sept. 13, 2016, 5 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51822/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2016, 5 a.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 > >