> On April 28, 2016, 10:54 p.m., Alexander Denissov wrote:
> > ambari-server/src/main/resources/common-services/HAWQ/2.0.0/service_advisor.py,
> >  line 110
> > <https://reviews.apache.org/r/44210/diff/3-5/?file=1363971#file1363971line110>
> >
> >     if we make isLocalHost utility function available in another utility, 
> > then there will be no need to pass stackAdvisor as parameter to all the 
> > functions in a ServiceAdvisor, as it promotes "spagetti-code", since 
> > StackAdvisor calls into ServiceAdvisor and having the call coming in the 
> > opposite directions is not desired.
> 
> Tim Thorpe wrote:
>     I have been asked to keep this patch as minimal as possible to avoid 
> destabilizing the code.  I could have refactored many of the functions into a 
> common file which both the stacks/stack_advisor.py and 
> stacks/service_advisor.py could use but this would result in many more 
> possible issues.  To play it safe I passed in the stackAdvisor.  In a 
> previous iteration, when I had refactored most of the stack advisor even for 
> the stack services like YARN, HDFS and HBASE, I found that sometimes I needed 
> information in say the YARN service_advisor.py that was only available in the 
> HDFS service_advisor.py.  In that case, I would create an HDFS service 
> advisor and call the required functions.  Now with all of those functions 
> being in the stackAdvisor, you can't do those sorts of things.  This means 
> that for some service, you might need a reference to the stackAdvisor.  
> Obviously it needs to be used sparingly.
>     
>     In the future I hope to come back to this code and give a better 
> separation between stack and service advisors.

Understood. I'm just afraid that the further rectoring might become problematic 
as it will require changes to the interface of the base service_advisor to 
remove passing in the stack advisor. Then we will have to refactor HAWq/PXF 
once again to consume the change. If the parameter is not there in the first 
place, service developers will take small steps to refactor pieces of logic 
they need from StackAdvisor into utils. However, we are OK with your approach 
as well.


- Alexander


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


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