----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43146/#review117802 -----------------------------------------------------------
Fix it, then Ship it! It may look like a lot of issues raised, but it's only around the boolean-ness of the new method. ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java (line 1714) <https://reviews.apache.org/r/43146/#comment179065> Odd syntax here. If the Impl returned non-null (and code implies it could be, but should it?), then this could be "boolean force = kerberosHelper.getForceToggle..." ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java (lines 502 - 503) <https://reviews.apache.org/r/43146/#comment179064> Impl can also return null. If null is valid, what does that return value imply? ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java (line 505) <https://reviews.apache.org/r/43146/#comment179066> I typically don't recommend a Boolean object return unless null is an option. ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java (lines 2087 - 2089) <https://reviews.apache.org/r/43146/#comment179063> Does the absence imply false? - Nate Cole On Feb. 3, 2016, 3:51 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43146/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2016, 3:51 p.m.) > > > Review request for Ambari, Jonathan Hurley, Nate Cole, and Robert Nettleton. > > > Bugs: AMBARI-14905 > https://issues.apache.org/jira/browse/AMBARI-14905 > > > Repository: ambari > > > Description > ------- > > Add an API directive to force toggling Kerberos if the cluster's security > type has not changed. > > This is useful for _retry_ attempts to enable or disable Kerberos when the > workflow progressed far enough to store the _new_ security type. > > Trying to enable Kerberos when the cluster's security type is already set to > `KERBEROS` will result in a successful response from the Ambari server but no > actions will be performed. Same for attempting to disable Kerberos when the > cluster's security type is already set to `NONE`. > > By forcing the operation using the `force_toggle_kerberos=true` directive, > the security type check is avoided, thus allowing the _retry_ operation to > proceed. > > Example: > ``` > PUT /api/v1/clusters/CLUSTER_NAME?force_toggle_kerberos=true > { > "Clusters" : { > "security_type" : "KERBEROS" > } > } > ``` > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/api/resources/ClusterResourceDefinition.java > b2be291 > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > 5010311 > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java > 5008d60 > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelperImpl.java > 63aa92d > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerImplTest.java > 5838001 > > ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java > 869d187 > > Diff: https://reviews.apache.org/r/43146/diff/ > > > Testing > ------- > > Manually tested (via API calls) > > # Local test results: > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 57:10.059s > [INFO] Finished at: Wed Feb 03 15:24:15 EST 2016 > [INFO] Final Memory: 71M/1409M > [INFO] > ------------------------------------------------------------------------ > > # Jenkins test results: PENDING > > > Thanks, > > Robert Levas > >
