----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51815/#review149205 -----------------------------------------------------------
Thanks for providing this patch. This is a feature in Blueprints that is sorely needed, and will be beneficial for Blueprints generally. I would ask, if possible, that some consideration be put into making the XML syntax for specifying conditions on dependencies a little more generic, and then making the two conditional types implemented here be subtypes of the generic type. My thinking here is that having this be just a little more flexible may benefit Blueprints in the future, as more conditional types may be required for different usage types. I agree completely that conditions based on configuration are the most common, and the two conditional types implemented are likely to be used in a variety of ways across the stacks, but it would be great if these were treated as specific sub-types of the condition, to make future conditions easier to add. Can you please add Jayush Lunia to this review list? I believe he's looking at some stack-level refactorings, so it would be good to get his input as well. Thanks. ambari-server/src/main/java/org/apache/ambari/server/state/DependencyConditionInfo.java (line 37) <https://reviews.apache.org/r/51815/#comment216756> I think this is a great idea, but I'd recommend a slightly more generic approach. Adding conditional dependencies is a great idea, and I would agree that by far that the most common case is configuration types. That being said, it might be a good idea to consider making this a little more generic, and then having the 'config-type-exists' and 'property-value-equals' conditions be specific cases of that generic type. It might be useful to add a new 'condition-type' field to DependencyConditionInfo, so that the stacks indicate the type of condition. This will make it easier to visually parse the stacks in the future, and allows for more conditional types to be added. Having this be a little more generic would allow future conditional types to be added without too much effort. Off the top of my head, I can't think of any, but I would expect that future use cases might open up possibilities of declaring conditions in a variety of ways, to simplify stack development and maintenance. ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintValidatorImplTest.java (line 87) <https://reviews.apache.org/r/51815/#comment216757> Minor issue: I couldn't find any usages of this field. Is this used in the unit tests? ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintValidatorImplTest.java (line 357) <https://reviews.apache.org/r/51815/#comment216755> Minor issue: Could you maybe rename this method, so that the intent of the test is a little clearer? Maybe something to indicate that the conditionally dependent component is missing. Sorry to nit-pick here, but that might make it easier to maintain this test over time. Thanks. - Robert Nettleton On Sept. 13, 2016, 9:28 p.m., Amruta Borkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51815/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2016, 9:28 p.m.) > > > Review request for Ambari, Shantanu Mundkur, Di Li, Juanjo Marron, Laszlo > Puskas, and Robert Nettleton. > > > Bugs: AMBARI-18355 > https://issues.apache.org/jira/browse/AMBARI-18355 > > > Repository: ambari > > > Description > ------- > > Currently stack definitions do not list conditional dependencies, adding > those to the stack definitions would make it easy to validate errors in case > of blueprint deployment. Please refer to document attached to Jira > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/state/DependencyConditionInfo.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/DependencyInfo.java > e3db662 > > ambari-server/src/main/java/org/apache/ambari/server/topology/BlueprintValidatorImpl.java > a5f33ff > > ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/metainfo.xml > 65d166a > > ambari-server/src/test/java/org/apache/ambari/server/topology/BlueprintValidatorImplTest.java > b1de8ef > > Diff: https://reviews.apache.org/r/51815/diff/ > > > Testing > ------- > > Written Junit test cases. Perfomred manual testing to check the Namenode HA > component dependency. Was able to proceed with the installation for valid > blueprint. and got validation error message while registering blueprint when > the Blueprint did not satisfy the conditional dependencies. > > > Thanks, > > Amruta Borkar > >
