----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37715/#review99033 -----------------------------------------------------------
Looks good overall, this will be very useful. ambari-server/src/main/java/org/apache/ambari/server/controller/StackServiceResponse.java (line 65) <https://reviews.apache.org/r/37715/#comment155853> Consistency of the service should be enforced when the service is parsed by the StackManager. At this point, the service has already been parsed and the server is running with a potentially invalid service definition. Throwing an exception here isn't really useful as there is no way that a caller could reasonably handle this exception. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackServiceResourceProvider.java (line 130) <https://reviews.apache.org/r/37715/#comment155854> could inline resource creation ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java (line 207) <https://reviews.apache.org/r/37715/#comment155855> This sentence seems to be incomplete. ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java (line 208) <https://reviews.apache.org/r/37715/#comment155856> missing description ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java (line 230) <https://reviews.apache.org/r/37715/#comment155858> Even after reading your response to questions about the need for this I am still not convinced that this is needed. Are you aware of a case where a request for this information can occur while the stack manager is still processing the stack. Unless the behavior has changed since I worked on it, this shouldn't ever be possible. None of the other merge methods behave this way so unless there really is a current need for this behavior I would recommend removing it. ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java (line 765) <https://reviews.apache.org/r/37715/#comment155860> Could simplify by removing this method and just have the one setter for service properties which takes a map which would also populate the list (or vice versa). ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java (line 777) <https://reviews.apache.org/r/37715/#comment155859> Why is this done lazily? ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java (line 782) <https://reviews.apache.org/r/37715/#comment155861> We should handle the case where duplicate properties are set on a service when the properties are set, not when they are retrieved. - John Speidel On Aug. 28, 2015, 1:09 p.m., Sebastian Toader wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37715/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2015, 1:09 p.m.) > > > Review request for Ambari, John Speidel, Robert Levas, and Robert Nettleton. > > > Bugs: AMBARI-7469 > https://issues.apache.org/jira/browse/AMBARI-7469 > > > Repository: ambari > > > Description > ------- > > Problem > In a stack there may be one or more services that should not be installable, > configurable, or managed via the Ambari web-based interface. Such a service > may need to be installed via some Ambari API call or manually. There is no > way to specify these “visibility” attributes within a service’s definition; > thus to “hide” a service, one-off code needs to be added to the different > Ambari facilities. > > Solution > Add visibility attributes to service definitions to describe how Ambari > should expose services via front-end facilities. The following Boolean > attributes should be settable by services: > - installable: indicates if Ambari can install the service - if not > installable, the service should be hidden from “add service” features of > Ambari > - managed: indicates if Ambari can start and stop the service - if not > managed, the service should be hidden from all views where management > operations can occur > - monitored: indicates if Ambari can monitor the service - if not monitored, > status information should not be displayed > > > These attributes are assumed to be trueif not specified in the service’s > metainfo.xml file, else > they are declared the a propertiesblock as follows: > <metainfo> > … > <services> > <service> > … > <properties> > <property> > <name>installable</name> > <value>false</name> > </property> > <property> > <name>managed</name> > <value>true</name> > </property> > </properties> > … > </service> > </services> > </metainfo> > > > Diffs > ----- > > ambari-server/pom.xml 01301da > > ambari-server/src/main/java/org/apache/ambari/server/controller/StackServiceResponse.java > d17fc32 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackServiceResourceProvider.java > 130129a > > ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java > e51eb21 > > ambari-server/src/main/java/org/apache/ambari/server/state/DuplicateServicePropertyException.java > PRE-CREATION > ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java > b476f0e > > ambari-server/src/main/java/org/apache/ambari/server/state/ServicePropertyInfo.java > PRE-CREATION > ambari-server/src/main/resources/properties.json 2dc1af5 > > ambari-server/src/test/java/org/apache/ambari/server/controller/StackServiceResponseTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackServiceResourceProviderTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/stack/ServiceModuleTest.java > 2737695 > > ambari-server/src/test/java/org/apache/ambari/server/state/ServiceInfoTest.java > df22acc > > ambari-server/src/test/java/org/apache/ambari/server/state/ServicePropertyInfoTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/37715/diff/ > > > Testing > ------- > > Manual tested on trunk: > - all three attributes with default value "true" are returned in case these > are not dedined in metainfo.xml > - merging between stack service and common services for these new attributes > > Local test results: > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 1:11.974s > [INFO] Finished at: Mon Aug 24 15:18:11 CEST 2015 > [INFO] Final Memory: 59M/994M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Sebastian Toader > >
