----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38183/#review99379 -----------------------------------------------------------
The patch looks fine overall, but I've come up with a few minor issues while looking through this a second time. I think we should try to resolve these open issues before merging this. We should ask jspeidel to review this as well. Thanks. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java (line 2113) <https://reviews.apache.org/r/38183/#comment156256> While looking through this again, I think I've found an issue that will require some refactoring of this patch: This initial approach looks fine, and will work for many cases in the stack configurations. One potential problem is that this patch adds the excluded config from the default stack definitions. This is correct for probably 90% of the cases for configuration, since the defaults (unless overridden by a Blueprint) will be valid. The only potential issue I see is if we run into a scenario where a property to be added in "excluded-config" requires topology substitution. If we ever need to support that, then this code will need to be refactored in order to use the PropertyUpdater mechanism to make sure that any properties that require hostname substitution will be updated as expected. I've just checked the current stack definitions, and it looks like the excluded-config properties in AMS and Falcon do not include topology information, so this isn't an issue currently. I do think it is worth considering for the future. We could potentially move forward with this patch as-is to fix the bug, and then consider a refactoring in the future if we run into excluded configs that require substitution. I'm opening this issue to track this discussion. Let's ask jspeidel to review this, to get his opinion as well. ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java (line 3687) <https://reviews.apache.org/r/38183/#comment156257> Could you also add a unit test to verify that a configuration override of one of these properties in the excluded configuration isn't overwritten by this change? If a Blueprint or cluster creation template sets this property explicitly, then the customer-defined value should be used, rather than the stack defaults. If you could add a test, or an additional assertion to this test to verify this, that would be great. - Robert Nettleton On Sept. 16, 2015, 4:40 p.m., Sandor Magyari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38183/ > ----------------------------------------------------------- > > (Updated Sept. 16, 2015, 4:40 p.m.) > > > Review request for Ambari, John Speidel, Robert Levas, and Robert Nettleton. > > > Bugs: AMBARI-13017 > https://issues.apache.org/jira/browse/AMBARI-13017 > > > Repository: ambari > > > Description > ------- > > PROBLEM > > Installing falcon with blueprint, oozie extensions are missing, hence causing > misconfigured installations that are using blueprint. > > SOLUTION > > This issue seems to be related not only for case of falcon but for every > service which has a configuration file for another service. > In case of falcon there is an oozie-site.xml (this contains oozie extensions) > in configuration of falcon service, which is marked as exclued-config-type in > service metainfo.xml. The same is true for AMBARI_METRICS which includes the > storm-site.xml. In case of a bluprint install these are excluded from > properties, which is fine because you can not add a config-type twice. The > solution would to add these properties from excluded-config-type at update > phase of properties in > BluprintConfigurationProcessor.doUpdateForClusterCreate. > We can apply this only specifically for FALCON but I dont see yet any > drawbacks of applying it for each service with excluded property. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java > 892cf32 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessorTest.java > a881472 > > Diff: https://reviews.apache.org/r/38183/diff/ > > > Testing > ------- > > Manually tested, creating new cluster with blueprint, unitests passed, > created a new testcase. > > > Thanks, > > Sandor Magyari > >
