> On Sept. 17, 2015, 1:37 p.m., Robert Nettleton wrote: > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintConfigurationProcessor.java, > > line 2113 > > <https://reviews.apache.org/r/38183/diff/1/?file=1065298#file1065298line2113> > > > > 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.
While we may need to handle this in the future, there are no current examples of excluded configuration that require topology substitution by the BlueprintConfigurationProcessor. If this requirement comes up later, we can easily refactor this code to support this as well. I'm ok with moving forward with this patch as-is. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38183/#review99379 ----------------------------------------------------------- On Sept. 18, 2015, 9:10 p.m., Sandor Magyari wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38183/ > ----------------------------------------------------------- > > (Updated Sept. 18, 2015, 9:10 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 > a97ca74 > > 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 > >
