> On April 29, 2016, 12:11 a.m., Alexander Denissov wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/stack/CommonServiceDirectory.java,
> >  line 56
> > <https://reviews.apache.org/r/44210/diff/5/?file=1364835#file1364835line56>
> >
> >     should we not have a convention at all and rely on property specified 
> > in metainfo.xml ?
> >     
> >     The service advisors can be called HAWQ200ServiceAdvisor for the 
> > service in common services and HDP23HAWQ200ServiceAdvisor for the version 
> > specific to HDP2.3 stack, if any.
> >     
> >     This is the argument about inheritance between advisors of different 
> > versions of service and stack.

I debated about this but I don't see what you really gain by allowing the 
advisor name or file name to be specified in the metainfo.xml.  Although the 
code there should be changed to serviceName + serviceVersion + 
"ServiceAdvisor".  This would support the situation you mentioned in your email 
about adding HAWQ 2.1.0 to common-services and it inheriting from 
HAWQ200ServiceAdvisor.

The file name should always be "service_advisor.py" much like we always have 
"stack_advisor.py".  The class name of the service advisor depends on whether 
it is from common-services or from the stack.  It should be 
<service_name><service_version>ServiceAdvisor in common-services and 
<stack_name><stack_version><service_name>ServiceAdvisor in the stack.


> On April 29, 2016, 12:11 a.m., Alexander Denissov wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/stack/StackServiceDirectory.java,
> >  line 65
> > <https://reviews.apache.org/r/44210/diff/5/?file=1364838#file1364838line65>
> >
> >     In case the service comes from the common services (like HAWQ) which 
> > can be versioned independently from the stack, I think we should include 
> > the service version:
> >     
> >     HDP23HAWQ200ServiceAdvisor when HDP2.3 uses HAWQ 2.0.0
> >     
> >     HDP23HAWQ210ServiceAdvisor when HDP2.3 uses HAWQ 2.1.0
> >     
> >     This example assumes that HAWQ 2.1.0 can be release before the next 
> > version of Ambari or stack can be released.

I'm not really sure how this would help.  Are you thinking that you'd want 2 
service advisor classes defined in HDP/2.3/services/HAWQ/service_advisor.py or 
2 service advisor files in HDP/2.3/services/HAWQ?  I don't really understand 
why you'd want to do either of those.  You are either using 200 or 210.  I 
don't think it really matters if that switch is defined at the 
service_advisor.py level or the metainfo.xml level.

So you'd define it as 
HDP23HAWQServiceAdvisor(service_advisor.HAWQ200ServiceAdvisor) or 
HDP23HAWQServiceAdvisor(service_advisor.HAWQ210ServiceAdvisor)


> On April 29, 2016, 12:11 a.m., Alexander Denissov wrote:
> > ambari-server/src/main/resources/common-services/HAWQ/2.0.0/service_advisor.py,
> >  line 33
> > <https://reviews.apache.org/r/44210/diff/5/?file=1364840#file1364840line33>
> >
> >     Should the name include the version, so the future versions can extend 
> > this:
> >     
> >     HAWQ200ServiceAdvisor ?
> >     
> >     Also applies to the file name, should we have 
> > HAWQ200_service_advisor.py ?

I agree for common-services the service version should be included.  I don't 
think there is a reason to have it in the file name.  I had a previous comment 
that recommended the file name always remain the same.  This is identical to 
the way stack_advisor.py is handled.


> On April 29, 2016, 12:11 a.m., Alexander Denissov wrote:
> > ambari-server/src/main/resources/common-services/HAWQ/2.0.0/service_advisor.py,
> >  line 46
> > <https://reviews.apache.org/r/44210/diff/5/?file=1364840#file1364840line46>
> >
> >     we have a case (not coded yet) that might require more flexible lgoic 
> > here for which we will need more metadata. When we do thin, we might extend 
> > the interface to take a list of current component layout.

I think I would need to see a bit more about what you are thinking.  I'm not 
sure that function would necessarily be the best place to have that logic.  
Either way, if in the future you need to change things, all you need to do is 
modify the stacks/stack_advisor.py to pass in the extra parameter(s), modify 
the stacks/service_advisor.py to accept the extra parameter(s) and then modify 
your service_advisor.py file(s) as well.


- Tim


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


On April 28, 2016, 4:36 p.m., Tim Thorpe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44210/
> -----------------------------------------------------------
> 
> (Updated April 28, 2016, 4:36 p.m.)
> 
> 
> Review request for Ambari, Alexander Denissov, bhuvnesh chaudhary, Jayush 
> Luniya, Oleksandr Diachenko, Sumit Mohanty, Srimanth Gunturi, and Yusaku Sako.
> 
> 
> Bugs: AMBARI-15226
>     https://issues.apache.org/jira/browse/AMBARI-15226
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Currently the stack advisor is defined under each stack version such as 
> HDP/2.3. The problem with this is that it restricts the services that can be 
> added to the stack. If a custom service is to be added, they would need to 
> modify the stack advisor. If the configuration recommendation and validation 
> can be done at the service level then the custom service could just include 
> their own recommendations and validations separately.
> 
> 
> Diffs
> -----
> 
>   ambari-server/src/main/assemblies/server.xml e1a4919 
>   
> ambari-server/src/main/java/org/apache/ambari/server/api/services/AmbariMetaInfo.java
>  df65010 
>   
> ambari-server/src/main/java/org/apache/ambari/server/api/services/stackadvisor/commands/StackAdvisorCommand.java
>  00c8696 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/StackServiceResponse.java
>  ca1968e 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/internal/StackServiceResourceProvider.java
>  6c6fa91 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/CommonServiceDirectory.java
>  636de37 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceDirectory.java
>  356adb1 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/ServiceModule.java 
> b7e09a9 
>   
> ambari-server/src/main/java/org/apache/ambari/server/stack/StackServiceDirectory.java
>  d27e52a 
>   ambari-server/src/main/java/org/apache/ambari/server/state/ServiceInfo.java 
> 5a2bf84 
>   
> ambari-server/src/main/resources/common-services/HAWQ/2.0.0/service_advisor.py
>  PRE-CREATION 
>   
> ambari-server/src/main/resources/common-services/PXF/3.0.0/service_advisor.py 
> PRE-CREATION 
>   ambari-server/src/main/resources/properties.json eac0dbd 
>   ambari-server/src/main/resources/stacks/HDP/2.0.6/services/stack_advisor.py 
> 1680f21 
>   ambari-server/src/main/resources/stacks/HDP/2.3/services/stack_advisor.py 
> d0ce196 
>   ambari-server/src/main/resources/stacks/service_advisor.py PRE-CREATION 
>   ambari-server/src/main/resources/stacks/stack_advisor.py 9979e7e 
>   ambari-server/src/test/python/stacks/2.3/common/test_stack_advisor.py 
> 2080c52 
> 
> Diff: https://reviews.apache.org/r/44210/diff/
> 
> 
> Testing
> -------
> 
> Ran all the non java unit tests.  
> 
> Total run:945
> Total errors:0
> Total failures:0
> 
> Manually configured HAWQ and PXF as part of the HDP 2.3 stack and made sure 
> their service advisors were called.
> 
> 
> Thanks,
> 
> Tim Thorpe
> 
>

Reply via email to