ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Nettleton" <rnettle...@hortonworks.com>
Subject Re: Review Request 37715: Add Visibility Attributes to Services
Date Tue, 25 Aug 2015 20:54:34 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37715/#review96410
-----------------------------------------------------------

Ship it!


Overall this looks fine to me.

I've highlighted some minor issues I've found, as well as some issues around concurrency locks.
 

I have a few questions:

1. What kind of testing was done with an actual cluster deployment? 
2. Do any services require this metadata?  It doesn't look like they've been updated here.
 Perhaps that is coming in a separate patch.
3. Do you think there might be a need for component-level properties at some point?  The service-level
properties make perfect sense, and will be very useful.  It may also be worth exploring having
properties like this at the component level, if we ever get use cases where we'd like to limit
the way the Ambari UI operates on a subset of components in a given service.  This isn't something
that should hold up a merge, but might be useful to consider for the future. 

Nice work!


ambari-server/src/main/java/org/apache/ambari/server/controller/StackServiceResponse.java
(line 26)
<https://reviews.apache.org/r/37715/#comment151746>

    Minor issue:
    
    In the Ambari team we tend to favor explicit imports, rather than using the "*".  
    
    I would recommend reverting this line back to the full list of imports.



ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java (line 35)
<https://reviews.apache.org/r/37715/#comment151751>

    This import needs to be fixed as well.



ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java (line 222)
<https://reviews.apache.org/r/37715/#comment151766>

    Why would this map need to be evaluated more than once?  
    
    I would expect that this list of properties is always statically-defined in the XML definitions
of a stack service.



ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java (line 767)
<https://reviews.apache.org/r/37715/#comment151754>

    This looks like a variant of double-checked locking to me.  I would recommend modifying
this to include the full method under the synchronized(this) block. 
    
    It looks like this code is basically safe, in terms of a potential concurrency issue,
but would be brittle to maintain in the future.



ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java (line 805)
<https://reviews.apache.org/r/37715/#comment151761>

    Since getServiceProperties() relies on the value of the servicePropertyMap in a synchronized
block, it might be a good idea to synchronize on the set here.  
    
    I don't see any obvious race conditions here, but just wanted to mention this as a precaution.



ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackServiceResourceProviderTest.java
(line 42)
<https://reviews.apache.org/r/37715/#comment151764>

    I thought that the Ambari team decided to standardize on using EasyMock as the mocking
framework for unit tests.
    
    That being said, I believe there is already a mix of both frameworks being used in the
ambari-server unit tests, so I wouldn't request that this be changed, but perhaps in the future
it might be desirable to add new tests with EasyMock?


- Robert Nettleton


On Aug. 24, 2015, 2:09 p.m., Sebastian Toader wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37715/
> -----------------------------------------------------------
> 
> (Updated Aug. 24, 2015, 2: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 2e16a59 
>   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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message