> On Jan. 19, 2015, 11 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java, > > line 379 > > <https://reviews.apache.org/r/30028/diff/1/?file=825033#file825033line379> > > > > Based on the type, this map seems to really be a map of type to > > configuurations? If so, how is this related to configurations which is > > being set independently of 'configurationTags'.
I agree with you, but someone implementing `org.apache.ambari.server.agent.ExecutionCommand#setConfigurationTags` feels differently. > On Jan. 19, 2015, 11 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java, > > line 396 > > <https://reviews.apache.org/r/30028/diff/1/?file=825033#file825033line396> > > > > why is configurations always being set to an empty map? If it is null, an NPE is thrown later - probably when the configurations are reconciled. This is how this sort of thing has been implemented by others in the past and fixed ExecutionCommand outside the scope of this patch. > On Jan. 19, 2015, 11 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java, > > line 399 > > <https://reviews.apache.org/r/30028/diff/1/?file=825033#file825033line399> > > > > why are config attributes always being set to an empty map? If it is null, an NPE is thrown later - probably when the configurations are reconciled. This is how this sort of thing has been implemented by others in the past and fixed ExecutionCommand outside the scope of this patch. > On Jan. 19, 2015, 11 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/AbstractServerAction.java, > > line 158 > > <https://reviews.apache.org/r/30028/diff/1/?file=825036#file825036line158> > > > > imo, it is generally better to return an empty collection > > (Collections.emptyMap()) than null so the caller doesn't need to check for > > null. Fixing in next patch... > On Jan. 19, 2015, 11 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/ADKerberosOperationHandler.java, > > line 545 > > <https://reviews.apache.org/r/30028/diff/1/?file=825037#file825037line545> > > > > why would this be null? Isn't there a default template in stack > > configuration? There is a default value in the stack, but I was under the impression not all paths looked there to set it if not set in the active configuration set. I will change if you beleive the template should never be null. > On Jan. 19, 2015, 11 a.m., John Speidel wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/ADKerberosOperationHandler.java, > > line 567 > > <https://reviews.apache.org/r/30028/diff/1/?file=825037#file825037line567> > > > > Gson instance should be instance field so that it is not created every > > time. Fixing in next patch. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30028/#review68612 ----------------------------------------------------------- On Jan. 19, 2015, 8:54 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30028/ > ----------------------------------------------------------- > > (Updated Jan. 19, 2015, 8:54 a.m.) > > > Review request for Ambari, Jaimin Jetly, John Speidel, Robert Nettleton, and > Tom Beerbower. > > > Bugs: AMBARI-9170 > https://issues.apache.org/jira/browse/AMBARI-9170 > > > Repository: ambari > > > Description > ------- > > The properties used to create accounts in an Active Directory, related to > principal creation, should be configurable such that a user may specify the > required fields and their values (with variable replacement). > > This may be done using a simple structure like XML or JSON, however a > template facility (like _Velocity_) may be more useful since conditional > paths may be built in. The template should be stored in the `kerberos-env` > configuration. > > An example of a need for a conditional path in a template is related to > _service_ accounts vs _user_ accounts. A _service_ account (such as > nn/_HOST@REALM) should have the `servicePrincipalName` field set to the > service's principal, where this value shouldn't be set for a _user_ account > (such as hdfs@REALM). > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java > f0e4e9c > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java > 6d0e159 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java > befd014 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/AbstractServerAction.java > 9882e73 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/ADKerberosOperationHandler.java > dce980d > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosOperationHandler.java > 49f573e > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerAction.java > b3592e6 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/kerberos/MITKerberosOperationHandler.java > 23eed35 > > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigHelper.java > e2e8c09 > > ambari-server/src/main/resources/common-services/KERBEROS/1.10.3-10/configuration/kerberos-env.xml > 0ecf6c0 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java > 2e79741 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java > c3418dc > > ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java > 8900a26 > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/ServerActionExecutorTest.java > 73f7be7 > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/ADKerberosOperationHandlerTest.java > aad325a > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosOperationHandlerTest.java > 40686d6 > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/KerberosServerActionTest.java > 5d72585 > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/kerberos/MITKerberosOperationHandlerTest.java > bd0e938 > > Diff: https://reviews.apache.org/r/30028/diff/ > > > Testing > ------- > > Manual testing in test cluster > > Updated unit tests > > #Jenkins Results > > Running > org.apache.ambari.server.serveraction.kerberos.ADKerberosOperationHandlerTest > Tests run: 10, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.711 sec > > Running > org.apache.ambari.server.serveraction.kerberos.MITKerberosOperationHandlerTest > Tests run: 9, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.422 sec > > Running > org.apache.ambari.server.serveraction.kerberos.KerberosServerActionTest > Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.167 sec > > Running > org.apache.ambari.server.serveraction.kerberos.MITKerberosOperationHandlerTest > Tests run: 9, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.422 sec > > Running org.apache.ambari.server.serveraction.ServerActionExecutorTest > Tests run: 5, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.761 sec > > Full ambari-server test suite > Tests run: 2548, Failures: 0, Errors: 0, Skipped: 15 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 01:00 h > [INFO] Finished at: 2015-01-19T13:30:31+00:00 > [INFO] Final Memory: 43M/531M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Robert Levas > >
